Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: cordova-weinre
    • Labels:

      Description

      Since Windows Phone does not provide any html debug possibilities it will be great to support it via Weinre remote debugger.

        Activity

        Hide
        pmuellr Patrick Mueller added a comment -

        I added the bug to the ChangeLog, did a build, updated npm and http://people.apache.org/~pmuellr/weinre/

        Think that should do it! You can close the pull requests.

        Show
        pmuellr Patrick Mueller added a comment - I added the bug to the ChangeLog, did a build, updated npm and http://people.apache.org/~pmuellr/weinre/ Think that should do it! You can close the pull requests.
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Merge looks great. Performed smoke testing of weinre apache master branch on Chrome, IE10 desktop, IE10 WP8 - no issues found. I'll perform additional testing and submit all problems found as separate issues (if any).

        Show
        sgrebnov Sergey Grebnov added a comment - Merge looks great. Performed smoke testing of weinre apache master branch on Chrome, IE10 desktop, IE10 WP8 - no issues found. I'll perform additional testing and submit all problems found as separate issues (if any).
        Hide
        pmuellr Patrick Mueller added a comment -

        Just realized that my merge --squash removed all of Sergey's commit comments. Here they are, prefix by the commit SHA from https://github.com/sgrebnov/incubator-cordova-weinre

        7e4eaf4 dirty corrections of target-script components
        a338415 console: fixes _defineGetter_ error
        d39a0a8 fixes metrics and inline styles
        029b989 partial fix for Properties
        dd42278 fixes Matched CSS Rules functionality
        523b923 allows browser specific hacks as part of target script
        56987c7 BrowserHacks improvements, made more universal
        936a692 removed empty lines in hacks.js
        21d0351 removed unnesessary console output
        1d8b53d fixes console ‘original’ property declaration issue
        d733dc6 logic to override \vendor\webkit files after getting fresh Web Inspector
        fa054f8 fixes isPropertyImplicit declaration (CSSStore.coffee) fa054f8
        9e7ebca ie compliant inspector inject script

        Show
        pmuellr Patrick Mueller added a comment - Just realized that my merge --squash removed all of Sergey's commit comments. Here they are, prefix by the commit SHA from https://github.com/sgrebnov/incubator-cordova-weinre 7e4eaf4 dirty corrections of target-script components a338415 console: fixes _ defineGetter _ error d39a0a8 fixes metrics and inline styles 029b989 partial fix for Properties dd42278 fixes Matched CSS Rules functionality 523b923 allows browser specific hacks as part of target script 56987c7 BrowserHacks improvements, made more universal 936a692 removed empty lines in hacks.js 21d0351 removed unnesessary console output 1d8b53d fixes console ‘original’ property declaration issue d733dc6 logic to override \vendor\webkit files after getting fresh Web Inspector fa054f8 fixes isPropertyImplicit declaration (CSSStore.coffee) fa054f8 9e7ebca ie compliant inspector inject script
        Hide
        pmuellr Patrick Mueller added a comment -

        I've merged the relevant changes in this commit:

        https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-weinre.git;a=commit;h=eea5bc59a1363e2cf2acc689835fb485bf63f405

        Thanks so much for doing this work, and putting up my curmudgeonly ways.

        Double-check that I got everything you needed. I'll do a build after you respond, and hopefully we can have some other window-ers try it out.

        Show
        pmuellr Patrick Mueller added a comment - I've merged the relevant changes in this commit: https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-weinre.git;a=commit;h=eea5bc59a1363e2cf2acc689835fb485bf63f405 Thanks so much for doing this work, and putting up my curmudgeonly ways. Double-check that I got everything you needed. I'll do a build after you respond, and hopefully we can have some other window-ers try it out.
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Patrick, I've created a new pull request as per your comments above.
        https://github.com/apache/incubator-cordova-weinre/pull/12

        PS .py files - the only reason of that change is to have py files Python 3.x compliant (2.x should be supported too); dropped from pull
        PPS. var->const - agree, dropped from pull

        Show
        sgrebnov Sergey Grebnov added a comment - Patrick, I've created a new pull request as per your comments above. https://github.com/apache/incubator-cordova-weinre/pull/12 PS .py files - the only reason of that change is to have py files Python 3.x compliant (2.x should be supported too); dropped from pull PPS. var->const - agree, dropped from pull
        Hide
        pmuellr Patrick Mueller added a comment -

        w/r/t pull/9

        I'm not going to accept the changes to the .py files, unless there's some reason I have to.
        I'm not going to accept the changes to the .js files involving var -> const

        Open up separate issues for these if you feel they must be made, and let me know if - lurking in all these .py and .js changes, there is actually interesting meaty stuff that we need - I didn't notice anything.

        Show
        pmuellr Patrick Mueller added a comment - w/r/t pull/9 I'm not going to accept the changes to the .py files, unless there's some reason I have to. I'm not going to accept the changes to the .js files involving var -> const Open up separate issues for these if you feel they must be made, and let me know if - lurking in all these .py and .js changes, there is actually interesting meaty stuff that we need - I didn't notice anything.
        Hide
        sgrebnov Sergey Grebnov added a comment -
        Show
        sgrebnov Sergey Grebnov added a comment - + added the following issue https://issues.apache.org/jira/browse/CB-1873
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Hi Patrick, I've addressed your review comments, please take a look
        https://github.com/apache/incubator-cordova-weinre/pull/9

        According to 'weinre.build/vendor' problem I've created 'vendor-override' folder as you suggested with the single InjectedScriptSource.js file. It is the only file required for the target script, other files could be overwritten (they only contains 'const->var' improvement which is not required for the target script). If we add support of non-webkit browsers for the client component then we will have to add all those files to the vendor-override folder, but for now let's keep it as simple as possible.

        I've also double checked "matchesSelector" - WP7(IE9) supports it only with ms vendor prefix.

        Show
        sgrebnov Sergey Grebnov added a comment - Hi Patrick, I've addressed your review comments, please take a look https://github.com/apache/incubator-cordova-weinre/pull/9 According to 'weinre.build/vendor' problem I've created 'vendor-override' folder as you suggested with the single InjectedScriptSource.js file. It is the only file required for the target script, other files could be overwritten (they only contains 'const->var' improvement which is not required for the target script). If we add support of non-webkit browsers for the client component then we will have to add all those files to the vendor-override folder, but for now let's keep it as simple as possible. I've also double checked "matchesSelector" - WP7(IE9) supports it only with ms vendor prefix.
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Upd. Still in my todo list. The plan is to get this done next week.

        Show
        sgrebnov Sergey Grebnov added a comment - Upd. Still in my todo list. The plan is to get this done next week.
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Patrick, thank you for detailed comments. Will update the code and send new PR.

        Show
        sgrebnov Sergey Grebnov added a comment - Patrick, thank you for detailed comments. Will update the code and send new PR.
        Hide
        pmuellr Patrick Mueller added a comment -

        comments on https://github.com/apache/incubator-cordova-weinre/pull/9/files


        all files in weinre.build/vendor/webkit need to be placed outside of that file tree. Problem is, the build system is set up to copy the Web Inspector files right from svn, right into weinre.build/vendor. So if we ever update the version of Web Inspector we're using, all these files will be overwritten.

        Suggest creating a new weinre.build/vendor-override directory, and the placing the files in there. The "get-vendor" bits of the ant build files should then be updated to `cp -R` those files over the top of weinre.build/vendor.

        So, the commit will actually have these files in twice; once in weinre.build/vendor and weinre.build/vendor-override.


        I'd like to see a monkeypatch of `Object.getPrototypeOf`, something like this:

        if (!Object.getPrototypeOf) {
            Object.getPrototypeOf = function(object) {
                if (!object.__proto__) throw new Error("This vm does not support __proto__))")
                return object.__proto__
            }
        }
        

        It goes somewhere in the target code, presumably very early. BrowserHacks?


        overzealous const -> var translation; remove

        • weinre.build/vendor/webkit/WebCore/inspector/front-end/SourceJavaScriptTokenizer.js:
        • weinre.build/vendor/webkit/WebCore/inspector/front-end/UglifyJS/parse-js.js
        • weinre.build/vendor/webkit/WebCore/inspector/front-end/UglifyJS/process.js

        empty line for: weinre.web/client/weinre/hacks.js

        Did your IDE do this for you? Prefer to not see these kinds of things, but survivable.


        weinre.web/modules/weinre/common/HookLib.coffee

        Don't spam the console. I don't think we need to print this message, just change it to return if @target is undefined. And the if statement looks like it's indented incorrectly.


        weinre.web/modules/weinre/target/BrowserHacks.coffee

        Checking the user agent seems fragile. Unless you can point me to something that says only IE-based browsers use it, or that none of the other browsers that we support use it (Android, iOS, Blackberry, and for good measure B2G or whatever).

        The `apply` function is poorly named, since it's used as a class method, and thus is an invocation of the method `apply` on a function, and so shadows `Function.prototype.apply`.

        And the BrowserHacks usage in Target.coffee doesn't really require this. Though weinre was built early-on as a "every module is a class" story, that's not needed any more. I'd just make this a plain old module, remove the class, make the `internetExplorerHacks` a top-level function in the module, and do the body of the `apply` function to the body of the module. Then Target.coffee will be changed to just do a "require 'BrowserHacks'" without the apply.


        weinre.web/modules/weinre/target/CSSStore.coffee

        The calculation of implicit looks wrong. Don't you need to invoke isPropertyImplicit, rather than just return it?

        I wonder if ms actually supports "matchesSelector" now; I think at the time, no one did, so I never bothered checking for it. Is now the time?


        weinre.web/modules/weinre/target/Console.coffee

        That replacement is not exactly correct. You are replacing a property with a function, so at least the invocations of the `original` method will need to be updated.

        I think I'd suggest replacing it with a defineProperties() invocation, as you did with some other changes.

        Show
        pmuellr Patrick Mueller added a comment - comments on https://github.com/apache/incubator-cordova-weinre/pull/9/files all files in weinre.build/vendor/webkit need to be placed outside of that file tree. Problem is, the build system is set up to copy the Web Inspector files right from svn, right into weinre.build/vendor. So if we ever update the version of Web Inspector we're using, all these files will be overwritten. Suggest creating a new weinre.build/vendor-override directory, and the placing the files in there. The "get-vendor" bits of the ant build files should then be updated to `cp -R` those files over the top of weinre.build/vendor. So, the commit will actually have these files in twice; once in weinre.build/vendor and weinre.build/vendor-override. I'd like to see a monkeypatch of `Object.getPrototypeOf`, something like this: if (!Object.getPrototypeOf) { Object.getPrototypeOf = function(object) { if (!object.__proto__) throw new Error("This vm does not support __proto__))") return object.__proto__ } } It goes somewhere in the target code, presumably very early. BrowserHacks? overzealous const -> var translation; remove weinre.build/vendor/webkit/WebCore/inspector/front-end/SourceJavaScriptTokenizer.js: weinre.build/vendor/webkit/WebCore/inspector/front-end/UglifyJS/parse-js.js weinre.build/vendor/webkit/WebCore/inspector/front-end/UglifyJS/process.js empty line for: weinre.web/client/weinre/hacks.js Did your IDE do this for you? Prefer to not see these kinds of things, but survivable. weinre.web/modules/weinre/common/HookLib.coffee Don't spam the console. I don't think we need to print this message, just change it to return if @target is undefined. And the if statement looks like it's indented incorrectly. weinre.web/modules/weinre/target/BrowserHacks.coffee Checking the user agent seems fragile. Unless you can point me to something that says only IE-based browsers use it, or that none of the other browsers that we support use it (Android, iOS, Blackberry, and for good measure B2G or whatever). The `apply` function is poorly named, since it's used as a class method, and thus is an invocation of the method `apply` on a function, and so shadows `Function.prototype.apply`. And the BrowserHacks usage in Target.coffee doesn't really require this. Though weinre was built early-on as a "every module is a class" story, that's not needed any more. I'd just make this a plain old module, remove the class, make the `internetExplorerHacks` a top-level function in the module, and do the body of the `apply` function to the body of the module. Then Target.coffee will be changed to just do a "require 'BrowserHacks'" without the apply. weinre.web/modules/weinre/target/CSSStore.coffee The calculation of implicit looks wrong. Don't you need to invoke isPropertyImplicit, rather than just return it? I wonder if ms actually supports "matchesSelector" now; I think at the time, no one did, so I never bothered checking for it. Is now the time? weinre.web/modules/weinre/target/Console.coffee That replacement is not exactly correct. You are replacing a property with a function, so at least the invocations of the `original` method will need to be updated. I think I'd suggest replacing it with a defineProperties() invocation, as you did with some other changes.
        Hide
        pmuellr Patrick Mueller added a comment -

        Great. Will take a look at it soon.

        w/r/t the CLA, I assume you did this fairly recently? I haven't seen a response yet, or I missed it. Can take a few days. Wanted to make sure you didn't send it two weeks ago or something.

        Feel free to convert the TODOs into new issues

        Show
        pmuellr Patrick Mueller added a comment - Great. Will take a look at it soon. w/r/t the CLA, I assume you did this fairly recently? I haven't seen a response yet, or I missed it. Can take a few days. Wanted to make sure you didn't send it two weeks ago or something. Feel free to convert the TODOs into new issues
        Hide
        sgrebnov Sergey Grebnov added a comment - - edited

        Forgot to mention - Apache secretary should have my signed CLA

        Show
        sgrebnov Sergey Grebnov added a comment - - edited Forgot to mention - Apache secretary should have my signed CLA
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Hello Patrick,

        Console and Elements (except Event Listeners) are working now. Could you please review the changes - I made a PR
        https://github.com/apache/incubator-cordova-weinre/pull/9

        PS. I reviewed changes in python files and they should compile fine on python 2.0. But if they are not, please reject the code and I'll switch them to original version.

        PPS. I've created a simple debug tool based on Weinre and Cordova, you can see it here http://sgrebnov.github.com/IeMobileDebugger/

        My TODO: fix Event Listeners (Elements), Resources, Network, Timeline

        Show
        sgrebnov Sergey Grebnov added a comment - Hello Patrick, Console and Elements (except Event Listeners) are working now. Could you please review the changes - I made a PR https://github.com/apache/incubator-cordova-weinre/pull/9 PS. I reviewed changes in python files and they should compile fine on python 2.0. But if they are not, please reject the code and I'll switch them to original version. PPS. I've created a simple debug tool based on Weinre and Cordova, you can see it here http://sgrebnov.github.com/IeMobileDebugger/ My TODO: fix Event Listeners (Elements), Resources, Network, Timeline
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Hi, small update from my side

        Was able to get console, metrics and inline styles working.

        Python. I believe the changes are 2.x compatible too, I will double check this (changed print->pint() and 'create' to 'open' for file api).

        The Doctype. Agree. I propose to get it working in IE9 mode first. Then we can consider with what the way to proceed - #1"tell ppl that they need a doctype" or #2"support quirks mode too".

        Show
        sgrebnov Sergey Grebnov added a comment - Hi, small update from my side Was able to get console, metrics and inline styles working. Python. I believe the changes are 2.x compatible too, I will double check this (changed print->pint() and 'create' to 'open' for file api). The Doctype. Agree. I propose to get it working in IE9 mode first. Then we can consider with what the way to proceed - #1"tell ppl that they need a doctype" or #2"support quirks mode too".
        Hide
        pmuellr Patrick Mueller added a comment -

        The defineGetter bit is one thing I suspected would be a problem. And there may be lurking _proto_ (or whatever) access as well.

        I've not yet had to modify existing WebKit bits, specifically the "injected script" bits, but of course if we have to, we have to.

        w/r/t Python 3 - is it still Python 2 compatible? I would like to continue to build on Mac/Lion for a while. I either need Python 2 compatibility, or an easy way to run Python 3 on Lion.

        The doctype thing, I guess, is allowing some JS APIs that you wouldn't ordinarily be able to get? How bad is it if we tell people "you NEED to have the html doctype"? I assume we'd be having to provide that answer ALL THE TIME to people, which would mean maybe it's worth the effort to get it to work in quirks mode.

        Show
        pmuellr Patrick Mueller added a comment - The defineGetter bit is one thing I suspected would be a problem. And there may be lurking _ proto _ (or whatever) access as well. I've not yet had to modify existing WebKit bits, specifically the "injected script" bits, but of course if we have to, we have to. w/r/t Python 3 - is it still Python 2 compatible? I would like to continue to build on Mac/Lion for a while. I either need Python 2 compatibility, or an easy way to run Python 3 on Lion. The doctype thing, I guess, is allowing some JS APIs that you wouldn't ordinarily be able to get? How bad is it if we tell people "you NEED to have the html doctype"? I assume we'd be having to provide that answer ALL THE TIME to people, which would mean maybe it's worth the effort to get it to work in quirks mode.
        Hide
        pmuellr Patrick Mueller added a comment -

        Awesome, thx Sergey.

        I assume you would like to contribute this to the project? If so, instructions on how to do this here:

        http://wiki.apache.org/cordova/ContributerWorkflow

        Technically, I think you can figure out the forking/pull request bits. The most important thing is the non-technical aspect - getting a CLA on file. See the first link in that file, follow the instructions there (eg, fax a signed version to some Apache secretary or something, etc).

        Show
        pmuellr Patrick Mueller added a comment - Awesome, thx Sergey. I assume you would like to contribute this to the project? If so, instructions on how to do this here: http://wiki.apache.org/cordova/ContributerWorkflow Technically, I think you can figure out the forking/pull request bits. The most important thing is the non-technical aspect - getting a CLA on file. See the first link in that file, follow the instructions there (eg, fax a signed version to some Apache secretary or something, etc).
        Hide
        sgrebnov Sergey Grebnov added a comment -

        Original repo https://github.com/apache/incubator-cordova-weinre
        The following was done so far
        1. Forked to https://github.com/sgrebnov/incubator-cordova-weinre
        2. Updated scripts to be python 3.x compliant
        3. Made proof of concept version by adding dummy stubs for unsupported be IE js classes and quick fixes - was able to connect to IE desktop and Windows Phone - https://vimeo.com/46852177

        The following works:
        1. Connection module
        2. Html element traversing
        3. Html modifications

        The following does not work:
        1. Console - there is an exception related not supported by IE getter definition _defineGetter_
        2. Element styles/computed styles
        3. Element properties
        4. Element metrics
        5. Event listener info

        Typical errors found
        1. webkit specific Element and Node classes
        2. _defineGetter_ not supported by IE
        3. html doc must have <!DOCTYPE html>, in another case it will run in quirks mode and won't work

        Show
        sgrebnov Sergey Grebnov added a comment - Original repo https://github.com/apache/incubator-cordova-weinre The following was done so far 1. Forked to https://github.com/sgrebnov/incubator-cordova-weinre 2. Updated scripts to be python 3.x compliant 3. Made proof of concept version by adding dummy stubs for unsupported be IE js classes and quick fixes - was able to connect to IE desktop and Windows Phone - https://vimeo.com/46852177 The following works: 1. Connection module 2. Html element traversing 3. Html modifications The following does not work: 1. Console - there is an exception related not supported by IE getter definition _ defineGetter _ 2. Element styles/computed styles 3. Element properties 4. Element metrics 5. Event listener info Typical errors found 1. webkit specific Element and Node classes 2. _ defineGetter _ not supported by IE 3. html doc must have <!DOCTYPE html>, in another case it will run in quirks mode and won't work

          People

          • Assignee:
            pmuellr Patrick Mueller
            Reporter:
            sgrebnov Sergey Grebnov
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development