Currently, the Web Console opens View Source when you click on a CSS link. It would be nicer to open the Style Editor.

...

Comment on attachment 661513 [details] [diff] [review] Patch v1.0 with tests Instead of making doCheck defer everything inside of an executeSoon, I think it would be clearer for people reading the test if callers of doCheck called in inside of executeSoon. >+ executeSoon(function() { >+ if (aCallback) { >+ aCallback(); >+ } >+ }); I think it would be clearer to have > if (aCallback) > executeSoon(aCallback); instead.

try push at : https://tbpl.mozilla.org/?tree=Try&rev=3d9b50bd2380 The test is mode complicated than the implementation because of the highly async nature of Style Editor. Even I am frustrated by the number of executeSoon needed :P

Josh's comment addressed :)

Comment on attachment 661515 [details] [diff] [review] Patch v1.1 with test doCheck is still a function wrapped in an executeSoon. Also, you could further reduce the visual complexity in doCheck by checking the false case first and doing an early return.

Try is green and test is now cleaned.

Further patch cleanup, following Josh's approach. Sorry for the spam.

Comment on attachment 661525 [details] [diff] [review] patch v1.3 Asking Rob for review :)

Comment on attachment 661525 [details] [diff] [review] patch v1.3 + viewSourceInStyleEditor: + function WC_viewSourceInStyleEditor(aSourceURL, aSourceLine) + { + let styleSheets = this.chromeWindow.content.window.document.styleSheets; + for each (let style in styleSheets) { could use for ... of here instead of the "for each in" construct. + } + SEM = null; not necessary. + let SEM = this.chromeWindow.StyleEditor.StyleEditorManager; + let win = SEM.getEditorForWindow(this.chromeWindow.content.window); + if (win) { + SEM.selectEditor(win, style, aSourceLine); + } + else { + this.chromeWindow.StyleEditor.openChrome(style, aSourceLine); + } + SEM = null; all of this could be replaced by a call to this.chromeWindow.StyleEditor.openChrome(style, aSourceLine). see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7473 in browser_webconsole_bug_782653_CSS_links_in_Style_Editor.js + * Contributor(s): + * Mihai Șucan <mihai.sucan@gmail.com> + * Girish Sharma <scrapmachines@gmail.com> + * there is no contributor block in a PD license. cancelling review for now. Mihai, would you prefer to wait for the web console remote protocol to land before letting this through? If so, Girish, could you base this on top of bug 768096?

(In reply to Rob Campbell [:rc] (:robcee) from comment #9) > Comment on attachment 661525 [details] [diff] [review] > patch v1.3 > > + viewSourceInStyleEditor: > + function WC_viewSourceInStyleEditor(aSourceURL, aSourceLine) > + { > + let styleSheets = this.chromeWindow.content.window.document.styleSheets; > + for each (let style in styleSheets) { > > could use for ... of here instead of the "for each in" construct. noted. > + } > + SEM = null; > > not necessary. This too, noted. > + let SEM = this.chromeWindow.StyleEditor.StyleEditorManager; > + let win = SEM.getEditorForWindow(this.chromeWindow.content.window); > + if (win) { > + SEM.selectEditor(win, style, aSourceLine); > + } > + else { > + this.chromeWindow.StyleEditor.openChrome(style, aSourceLine); > + } > + SEM = null; > > all of this could be replaced by a call to > this.chromeWindow.StyleEditor.openChrome(style, aSourceLine). No, it does not work. If a Style Editor is already opened, then it just focuses that. It does not switch the style sheet and focuses the line. This cann be seen in the method itself, that if it find the |win| to be not null, it just selects it and returns it. It does not focuses a different style sheet nor does jumps to the required line. > + * Contributor(s): > + * Mihai Șucan <mihai.sucan@gmail.com> > + * Girish Sharma <scrapmachines@gmail.com> > + * > > there is no contributor block in a PD license. The Contributor block was present in other tests of web console, thus I added it. too. > > cancelling review for now. Mihai, would you prefer to wait for the web > console remote protocol to land before letting this through? > > If so, Girish, could you base this on top of bug 768096? Ok, then I will wait for Mihai's reply before addressing your comments.

Comment on attachment 661525 [details] [diff] [review] patch v1.3 Review of attachment 661525 [details] [diff] [review]: ----------------------------------------------------------------- Thank you Girish! Patch looks really good. I only have one comment, see below. Btw, the test is mind bending. :) Rob, thank you for reviewing the patch. This can land before bug 768096 - it shouldn't cause any problems. ::: browser/devtools/webconsole/HUDService.jsm @@ +1074,5 @@ > + */ > + viewSourceInStyleEditor: > + function WC_viewSourceInStyleEditor(aSourceURL, aSourceLine) > + { > + let styleSheets = this.chromeWindow.content.window.document.styleSheets; This is not the correct way to get to the content window of the current Web Console instance. Please do something like this.tab.linkedBrowser.contentWindow. Also please add a comment that warns this method breaks the client-server boundaries. This method can produce unexpected results when the Web Console is connected to a different server.

(In reply to Mihai Sucan [:msucan] from comment #11) > Comment on attachment 661525 [details] [diff] [review] > patch v1.3 > > Review of attachment 661525 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you Girish! Patch looks really good. I only have one comment, see > below. Btw, the test is mind bending. :) > > Rob, thank you for reviewing the patch. This can land before bug 768096 - it > shouldn't cause any problems. > > ::: browser/devtools/webconsole/HUDService.jsm > @@ +1074,5 @@ > > + */ > > + viewSourceInStyleEditor: > > + function WC_viewSourceInStyleEditor(aSourceURL, aSourceLine) > > + { > > + let styleSheets = this.chromeWindow.content.window.document.styleSheets; > > This is not the correct way to get to the content window of the current Web > Console instance. > > Please do something like this.tab.linkedBrowser.contentWindow. Also please > add a comment that warns this method breaks the client-server boundaries. > This method can produce unexpected results when the Web Console is connected > to a different server. Oh, damn. I totally forgot about that. Should I have everything under a try catch ? So that if the Style Editor is not available (on Mobile) then it would simply go to catch ? As for if the client and server both are desktops, then it would simply not find the style in the stylesheets. Comment is also necessary .

(In reply to Girish Sharma [:Optimizer] from comment #12) > Oh, damn. I totally forgot about that. Should I have everything under a try > catch ? > > So that if the Style Editor is not available (on Mobile) then it would > simply go to catch ? As for if the client and server both are desktops, then > it would simply not find the style in the stylesheets. SE is always available even if you connect to a mobile. Server is one thing. You are changing the client stuff. SE shouldn't break if we give it unknown stylesheets or other something else wrong. Obviously, in practice we will need to see what has to be done to fix stuff. At this point, don't bother. Maybe file a follow-up bug about this method breaking the client-server boundaries and mention it in the comment.

Addresses comments of both Rob and Mihai

Comment on attachment 663495 [details] [diff] [review] Patch v1.4 + this.selectedStyleSheetIndex = aEditor.styleSheetIndex; this._view.activeSummary = summary; } else { + this.selectedStyleSheetIndex = aEditor.styleSheetIndex; same line in both places. you can put that outside the "if (setCaret)" statement. r+ with that change. I don't know if you should rebase this on the web console remote patches. Ask mihai.

now rebased on top of webconsole remote debugging patches. Addressed Rob's comments. Land it right after bug 768096 lands. (the other way round does not work)

As per the discussion with Mihai, this bug is good to go before bug 768096 as that is on the verge of missing Firefox 18. Please land this bug in time so that it gets into m-c before merge :) (We can wait till today evening also, if someone is available to land later tonight)

I work on m-c :( so I had to merge fx-team locally. Thus so many patches in try. https://tbpl.mozilla.org/?tree=Try&rev=8a322c5b1cb9

The test timed out once on Mac OSX 10.7 debug build, which I was not able to reproduce even with extra oth tests on the same system. I guess it is a very hard to reproduce intermittent. I looked into the code and could find anything that I am missing. I leave it upto you people to either land this before the merge tomorrow, or wait for a proper fix, if at all needed. Thanks for the time and sorry for all the bug mail spam.

(In reply to Girish Sharma [:Optimizer] from comment #19) > The test timed out once on Mac OSX 10.7 debug build, which I was not able to > reproduce even with extra oth tests on the same system. I guess it is a very > hard to reproduce intermittent. > > I looked into the code and could find anything that I am missing. > > I leave it upto you people to either land this before the merge tomorrow, or > wait for a proper fix, if at all needed. > > Thanks for the time and sorry for all the bug mail spam. The test failed once in 53 runs in a single platform, so in other words it is a rare intermittent failure. There is some sort of race in that test that we should figure out and fix, but not something uncommon in our test suite. We can certainly land this as it is and fix the test later. For future reference, you don't need more than a handful of test runs to see if you have a perma-orange or the regular variety :-)

Comment on attachment 668781 [details] [diff] [review] Rebased on top of latest fx-team Review of attachment 668781 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/test/browser_webconsole_bug_782653_CSS_links_in_Style_Editor.js @@ +14,5 @@ > +function test() { > + addTab(TEST_URI); > + browser.addEventListener("load", function onLoad() { > + browser.removeEventListener("load", onLoad, true); > + openConsole(null, testViewSource); Potential cause for the rare intermittent orange: you load the URL, then you openConsole() - events might fire at unexpected times. I would recommend something like: addTab("data:text/html,<p>foo"); wait-for-load openConsole content.location = TEST_URI; wait-for-load then start your testing

(In reply to Mihai Sucan [:msucan] from comment #21) > Potential cause for the rare intermittent orange: you load the URL, then you > openConsole() - events might fire at unexpected times. I would recommend > something like: > > addTab("data:text/html,<p>foo"); > wait-for-load > openConsole > content.location = TEST_URI; > wait-for-load then start your testing All the other tests were doing it like that only so I followed. Moreover, the one time failure that I got was while checking if the correct line is selected or not. Which means the Console ans Style Editor both opened up fine. I do not intend to push any more try pushes for this bug as even if someone else requests multiple oths without my knowledge, I will be blamed.