Closed Bug 1148893 Opened 5 years ago Closed 5 years ago

Clean up tests and make them use improved initialization methods

Categories

(DevTools :: Style Editor, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: sjakthol, Assigned: sjakthol)

Details

Attachments

(22 files, 3 obsolete files)

3.38 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
2.80 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
3.34 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
6.48 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
4.50 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
2.73 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
4.55 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
6.79 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
2.46 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
2.38 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.96 KB, patch
sjakthol
: review+
Details | Diff | Splinter Review
6.43 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
1.87 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.42 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.94 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
4.04 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.88 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
1.67 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
8.81 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.60 KB, patch
sjakthol
: review+
Details | Diff | Splinter Review
2.77 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.74 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
Bug 1147765 improved handling of asynchronous initialization tasks and introduced a new helper method openStyleEditorForURL to initialize style editor in tests. The new method is a lot more reliable than the currently used methods and thus the tests should be ported to use it.

While at it, it might be a good idea to make them use the latests test methods such that add_task, promises and content scripts.

Patches upcoming.
This patch makes the promise that's resolved once style sheet selection process completes available to the caller. It'll be useful for tests and other code that needs to select an editor and wait for the operation to complete.
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8585205 - Flags: review?(ejpbruel)
This patch fixes "Error: Unknown sheet source" rejection triggered by bunch of tests.

As the code runs inside a Task, it replaces .then handlers and three different rejection handlers (which fail to catch this rejection) with yields and the Task level rejection handler. It'll catch the rejection and make the tests work without thisTestLeaksUncaughtRejectionsAndShouldBeFixed notices.
Attachment #8585207 - Flags: review?(ejpbruel)
This patch adds helpers required to use the devtools wide frame-script-utils.js to head.js. The code itself is already used in other tests like [1] and [2].

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/head.js#448-494
[2] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/head.js#699-744

These three patches lay the ground for cleaning up the actual tests.
Attachment #8585209 - Flags: review?(ejpbruel)
Attachment #8585205 - Flags: review?(ejpbruel) → review+
Attachment #8585207 - Flags: review?(ejpbruel) → review+
Comment on attachment 8585209 [details] [diff] [review]
bug-1148893-3-frame-script-utils.patch

Review of attachment 8585209 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleeditor/test/head.js
@@ +188,5 @@
> + */
> +function waitForContentMessage(name) {
> +  let mm = gBrowser.selectedBrowser.messageManager;
> +
> +  let def = promise.defer();

I personally prefer new Promise over promise.defer here, but that's a style issue, and I don't care strongly about it.
Attachment #8585209 - Flags: review?(ejpbruel) → review+
Let's check these three in now.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25bde88c5fcc (the patches are there somewhere down the graph).
Keywords: checkin-needed
Whiteboard: [leave open]
This patch cleans up test browser_styleeditor_reload.js.

The test was testing two different scenarios:
1) page is reloaded => selected sheet and caret position are remembered
2) page is navigated => selected sheet and caret position are NOT remembered

I thought it'd be better to split these into two separate tests and did just that.

The diff might look a bit intimidating but when applied it should be pretty obvious what the tests are doing. Here's few notes that might help with the review:
- openStyleEditorForURL waits for every sheet to be loaded before resolving
- selectStyleSheet waits for the selected sheet source editor to be ready; no need to listen for "editor-selected" events anymore
- "stylesheets-reset" event comes after every sheet is loaded after reload / navigation; the source editor for the initially selected editor is not yet available
- getSourceEditor resolves once the actual sourceEditor is ready
Attachment #8587312 - Flags: review?(ejpbruel)
This patch cleans up browser_styleeditor_sv_keynav.js.

Here's a quick outline of the test:
- open and wait for the first source editor to be ready
- click on the active style sheet in the sidebar, press a few keys and check that focus moves correctly
- at the end the second to last sheet is focused and ENTER selects it
- wait for "stylesheet-selected" and check that the editor is focused; the event is only emitted after source editor is loaded
Attachment #8587315 - Flags: review?(ejpbruel)
This patch cleans up browser_styleeditor_selectstylesheet.js

The first patch of this bug simplifies this a lot as there's no need to wait for events anymore.
Attachment #8587333 - Flags: review?(ejpbruel)
Attached patch bug-1148893-7-pretty.patch (obsolete) — Splinter Review
This patch cleans up browser_styleeditor_pretty.js.

Here's a quick outline of the test:
- open style editor, an originally minified source is selected
- check that the source has been minified
- select another, non-minified source
- check that the source is left untouched

Try run with the four patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dbffbd810d0
Attachment #8587343 - Flags: review?(ejpbruel)
Attached patch bug-1148893-8-nostyle.patch (obsolete) — Splinter Review
This patch cleans up browser_styleeditor_nostyle.js.

The test does following:
- open a tab without waiting for it to load
- open style editor
- wait for tab and the editor to load
- check that the "No style sheets" notice is visible

Brian, I'm asking you to review a few patches to share the load a bit. The patches are fully independent (one patch touches one test) and don't change the actual functionality so it should be possible for both of you to review these simultaneously.
Attachment #8587545 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_new.js

Just removed some global variables and replaced for-each-in loop with for-of loop. Also, the "Unknown sheet source" rejections are no longer a problem as fix for them landed few days ago.
Attachment #8587546 - Flags: review?(bgrinstead)
This patch contains few minor changes to multiple tests: adding add_task, removing waitForExplicitFinish (handled by add_task) and replacing old initialization methods with a more reliable openStyleEditorForURL.
Attachment #8587547 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_loading.js

The test is very much similar to browser_styleeditor_nostyle.js and the diffs looks quite identical.
Attachment #8587552 - Flags: review?(bgrinstead)
Comment on attachment 8587545 [details] [diff] [review]
bug-1148893-8-nostyle.patch

Review of attachment 8587545 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleeditor/test/browser_styleeditor_nostyle.js
@@ +12,5 @@
>    // launch Style Editor right when the tab is created (before load)
>    // this checks that the Style Editor still launches correctly when it is opened
>    // *while* the page is still loading. The Style Editor should not signal that
>    // it is loaded until the accompanying content page is loaded.
> +  let tabAdded = addTab(TESTCASE_URI);

Why not use

  let { panel } = yield openStyleEditorForURL(TESTCASE_URI);

As a replacement for this whole preamble? (up until `let { panelWindow } = panel`)
Attachment #8587545 - Flags: review?(bgrinstead) → feedback+
Comment on attachment 8587546 [details] [diff] [review]
bug-1148893-9-new.patch

Review of attachment 8587546 [details] [diff] [review]:
-----------------------------------------------------------------

looks good!  Please update commit message to include reviewer before landing
Attachment #8587546 - Flags: review?(bgrinstead) → review+
Comment on attachment 8587547 [details] [diff] [review]
bug-1148893-10-multiple-minor-changes.patch

Review of attachment 8587547 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleeditor/test/browser_styleeditor_highlight-selector.js
@@ -6,5 @@
> -//
> -// Whitelisting this test.
> -// As part of bug 1077403, the leaking uncaught rejection should be fixed. 
> -//
> -thisTestLeaksUncaughtRejectionsAndShouldBeFixed("Error: Unknown sheet source");

I'm assuming this isn't the case anymore - a try push should confirm
Attachment #8587547 - Flags: review?(bgrinstead) → review+
Comment on attachment 8587552 [details] [diff] [review]
bug-1148893-11-loading.patch

Review of attachment 8587552 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleeditor/test/browser_styleeditor_loading.js
@@ +11,1 @@
>    // launch Style Editor right when the tab is created (before load)

Interesting, I guess maybe this is what the bug-1148893-8-nostyle.patch was going after.  Can you pull this into a head.js function that includes this comment and a descriptive name so both of these tests can use it?  It could resolve with { toolbox, panel }
Attachment #8587552 - Flags: review?(bgrinstead) → feedback+
Thanks for the quick reviews and feedback.

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8587545 [details] [diff] [review]
> bug-1148893-8-nostyle.patch
> 
> Review of attachment 8587545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_nostyle.js
> @@ +12,5 @@
> >    // launch Style Editor right when the tab is created (before load)
> >    // this checks that the Style Editor still launches correctly when it is opened
> >    // *while* the page is still loading. The Style Editor should not signal that
> >    // it is loaded until the accompanying content page is loaded.
> > +  let tabAdded = addTab(TESTCASE_URI);
> 
> Why not use
> 
>   let { panel } = yield openStyleEditorForURL(TESTCASE_URI);
> 
> As a replacement for this whole preamble? (up until `let { panelWindow } =
> panel`)
I think that is a good idea. I just didn't want to remove it as I don't really know if there's some specific reason for using that kind of initialization sequence. Will change.

(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8587552 [details] [diff] [review]
> bug-1148893-11-loading.patch
> 
> Review of attachment 8587552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_loading.js
> @@ +11,1 @@
> >    // launch Style Editor right when the tab is created (before load)
> 
> Interesting, I guess maybe this is what the bug-1148893-8-nostyle.patch was
> going after.  Can you pull this into a head.js function that includes this
> comment and a descriptive name so both of these tests can use it?  It could
> resolve with { toolbox, panel }

I think it's enough to use that special initialization sequence only in this test. It ought to provide enough coverage to check that style editor doesn't break if it's opened while the page is still loading.

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8587547 [details] [diff] [review]
> bug-1148893-10-multiple-minor-changes.patch
> 
> Review of attachment 8587547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/devtools/styleeditor/test/browser_styleeditor_highlight-selector.js
> @@ -6,5 @@
> > -//
> > -// Whitelisting this test.
> > -// As part of bug 1077403, the leaking uncaught rejection should be fixed. 
> > -//
> > -thisTestLeaksUncaughtRejectionsAndShouldBeFixed("Error: Unknown sheet source");
> 
> I'm assuming this isn't the case anymore - a try push should confirm

Yes, fixed by https://hg.mozilla.org/integration/fx-team/rev/91d52c8a63a9 (will push to try after updating the patches).
Here's the updated patch with the normal initialization sequence. The special case of page still loading when style editor opens is now only tested in browser_stylesheet_loading.js.
Attachment #8587545 - Attachment is obsolete: true
Attachment #8587610 - Flags: review?(bgrinstead)
Attachment #8587610 - Flags: review?(bgrinstead) → review+
Comment on attachment 8587552 [details] [diff] [review]
bug-1148893-11-loading.patch

This should be good now as this test is only one using the different initialization.

Here's a try run with all of the patches in this bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f418ae7bb3ad
Attachment #8587552 - Flags: review?(bgrinstead)
Comment on attachment 8587552 [details] [diff] [review]
bug-1148893-11-loading.patch

Review of attachment 8587552 [details] [diff] [review]:
-----------------------------------------------------------------

Is addTabAndCheckOnStyleEditorAdded still used even after all of the refactors?  If not, let's not forget to remove it from head.js
Attachment #8587552 - Flags: review?(bgrinstead)
Attachment #8587552 - Flags: review+
Attachment #8587552 - Flags: feedback+
Comment on attachment 8587312 [details] [diff] [review]
bug-1148893-4-reload.patch

Review of attachment 8587312 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few questions. If you can answer those, r+

::: browser/devtools/styleeditor/test/browser_styleeditor_reload.js
@@ +24,3 @@
>  
> +  info("Waiting for sheets to be loaded after reload.");
> +  yield ui.once("stylesheets-reset");

This should work because stylesheets-reset is not emitte until after both editors have been reloaded, right?

@@ +29,3 @@
>  
> +  info("Waiting for source editor to be ready.");
> +  yield ui.editors[1].getSourceEditor();

Why do we need to wait for getSourceEditor here? Is ui.selectedEditor not set to ui.editors[1] before this promise resolves?
Attachment #8587312 - Flags: review?(ejpbruel) → review+
Comment on attachment 8587315 [details] [diff] [review]
bug-1148893-5-sv-keynav.patch

Review of attachment 8587315 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8587315 - Flags: review?(ejpbruel) → review+
Attachment #8587333 - Flags: review?(ejpbruel) → review+
Comment on attachment 8587343 [details] [diff] [review]
bug-1148893-7-pretty.patch

Review of attachment 8587343 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed

::: browser/devtools/styleeditor/test/browser_styleeditor_pretty.js
@@ +8,4 @@
>  
>  const TESTCASE_URI = TEST_BASE_HTTP + "minified.html";
>  
> +const PRETTIFIED_SOURCE = "body\{\r?\n\tbackground\:white;\r?\n\}\r?\n\r?\ndiv\{\r?\n\tfont\-size\:4em;\r?\n\tcolor\:red\r?\n\}\r?\n\r?\nspan\{\r?\n\tcolor\:green;\r?\n\}\r?\n";

Hmmm, I'm not a big fan of inlining source like this. Suppose this test fails for some reason. It would be annoying to have to infer what the pretty printed source is expected to look like from this. Similarly, if the test turns out to be bugged for some reason, this is annoying to make changes in. How about putting this into a helper file? Or at the very least, put each string after a newline on its own line?

Since this inline string was already in the test before you refactored it, im not going to r- over this.

@@ -36,5 @@
> -    ok(prettifiedSourceRE.test(aEditor.sourceEditor.getText()),
> -       "minified source has been prettified automatically");
> -    editorTestedCount++;
> -    let summary = gUI.editors[1].summary;
> -    EventUtils.synthesizeMouseAtCenter(summary, {}, gPanelWindow);

Is this now unnecessary?
Attachment #8587343 - Flags: review?(ejpbruel) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #24)
> Comment on attachment 8587312 [details] [diff] [review]
> bug-1148893-4-reload.patch
> 
> Review of attachment 8587312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few questions. If you can answer those, r+
> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_reload.js
> @@ +24,3 @@
> >  
> > +  info("Waiting for sheets to be loaded after reload.");
> > +  yield ui.once("stylesheets-reset");
> 
> This should work because stylesheets-reset is not emitte until after both
> editors have been reloaded, right?
Yes, that's correct.

> @@ +29,3 @@
> >  
> > +  info("Waiting for source editor to be ready.");
> > +  yield ui.editors[1].getSourceEditor();
> 
> Why do we need to wait for getSourceEditor here? Is ui.selectedEditor not
> set to ui.editors[1] before this promise resolves?

ui.selectedEditor is set to ui.editors[1] before "stylesheets-reset" event is emitted. That yield ensures that we have the source editor for line 35 where it's used. The selected editor assertion could be moved above the yield you pointed but I just happened to write it in that order and it works either way.
(In reply to Eddy Bruel [:ejpbruel] from comment #26)
> Comment on attachment 8587343 [details] [diff] [review]
> bug-1148893-7-pretty.patch
> 
> Review of attachment 8587343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed
> 
> ::: browser/devtools/styleeditor/test/browser_styleeditor_pretty.js
> @@ +8,4 @@
> >  
> >  const TESTCASE_URI = TEST_BASE_HTTP + "minified.html";
> >  
> > +const PRETTIFIED_SOURCE = "body\{\r?\n\tbackground\:white;\r?\n\}\r?\n\r?\ndiv\{\r?\n\tfont\-size\:4em;\r?\n\tcolor\:red\r?\n\}\r?\n\r?\nspan\{\r?\n\tcolor\:green;\r?\n\}\r?\n";
> 
> Hmmm, I'm not a big fan of inlining source like this. Suppose this test
> fails for some reason. It would be annoying to have to infer what the pretty
> printed source is expected to look like from this. Similarly, if the test
> turns out to be bugged for some reason, this is annoying to make changes in.
> How about putting this into a helper file? Or at the very least, put each
> string after a newline on its own line?
> 
> Since this inline string was already in the test before you refactored it,
> im not going to r- over this.
We can't really move the strings outside the test file as it's needed for building the RegExp. But a better formatting is definitely possible. I'll update the patch.

> @@ -36,5 @@
> > -    ok(prettifiedSourceRE.test(aEditor.sourceEditor.getText()),
> > -       "minified source has been prettified automatically");
> > -    editorTestedCount++;
> > -    let summary = gUI.editors[1].summary;
> > -    EventUtils.synthesizeMouseAtCenter(summary, {}, gPanelWindow);
> 
> Is this now unnecessary?
Yes, the patch uses selectStyleSheet which makes the process of selecting the sheet a lot simpler (single yield vs. clicking on DOM elements and waiting for events). Selecting a sheet by clicking its name is tested elsewhere so it doesn't really make sense here.
This version adds some newlines to the regular expressions. It's still quite ugly but I added the content it's trying to match as a comment next to the expression which should clear things up.

Carrying r+ over.
Attachment #8587343 - Attachment is obsolete: true
Attachment #8589142 - Flags: review+
Following patches are ready to land:
bug-1148893-11-loading.patch
bug-1148893-10-multiple-minor-changes.patch
bug-1148893-9-new.patch
bug-1148893-8-nostyle.patch
bug-1148893-7-pretty.patch
bug-1148893-6-selectstylesheet.patch
bug-1148893-5-sv-keynav.patch
bug-1148893-4-reload.patch

Let's see how this goes before continuing with the remaining tests.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f418ae7bb3ad
Keywords: checkin-needed
This patch cleans up browser_styleeditor_inline_friendly_names.js.

The test was already in quite a good shape and I just made follwing changes:
- replaced .then chain with add_task
- used frame script to reload and navigate the page
- used "stylesheets-reset" event to detect readyness of the tool after navigation and reload
- removed global reference to the ui object

Also, the current version closed the toolbox and tab before navigating to another page and then opened it again to check that the names were forgotten. I removed the extra close - open hassle from the test as usually things are forgotten when the tool is closed but simple navigation requires the code actively to reset the stored information.
Attachment #8592709 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_import_rule.js.
Attachment #8592710 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_import.js.
Attachment #8592711 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_filesave.js.
Attachment #8592713 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_enabled.js.
Attachment #8592714 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_bug_851132_middle_click.js.
Attachment #8592716 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_bug_740541_iframes.js.
Attachment #8592717 - Flags: review?(bgrinstead)
This patch cleans up browser_styleeditor_autocomplete.js.

This is slightly more involved than the previous patches. The current version bounces between testState and checkState incrementing the data index at each iteration and call finish() when the last data item is handled. To make this compatible with add_task I changed those methods return promises and the test loop trough the indexes yielding on those promises.

I also separated test for disabled autocomplete into browser_styleeditor_autocomplete-disabled.js.
Attachment #8592718 - Flags: review?(bgrinstead)
Attached patch bug-1148893-20-resize.patch (obsolete) — Splinter Review
This patch cleans up browser_styleeditor_sv-resize.js.

While working on that I noticed the test did nothing useful as it tried to resize an iframe window inside main browser window which just doesn't work. So, I edited the test to change the toolbox into a window host and resized it instead.

The test was also nesting multiple levels of executeSoon / waitForFocus which don't seem to be relevant anymore. I removed those but if it causes issues they can always be restored.

----

This is the last patch that touches any of the tests. No need to hurry with the reviews. Once these land and stick I'll remove the redundant methods from head.js.

Try with all patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb1754de12fa
Attachment #8592723 - Flags: review?(bgrinstead)
Attachment #8592709 - Flags: review?(bgrinstead) → review+
Attachment #8592710 - Flags: review?(bgrinstead) → review+
Attachment #8592711 - Flags: review?(bgrinstead) → review+
Attachment #8592713 - Flags: review?(bgrinstead) → review+
Comment on attachment 8592714 [details] [diff] [review]
bug-1148893-16-enabled.patch

Review of attachment 8592714 [details] [diff] [review]:
-----------------------------------------------------------------

Nice
Attachment #8592714 - Flags: review?(bgrinstead) → review+
Attachment #8592717 - Flags: review?(bgrinstead) → review+
Attachment #8592716 - Flags: review?(bgrinstead) → review+
Comment on attachment 8592723 [details] [diff] [review]
bug-1148893-20-resize.patch

Review of attachment 8592723 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea.. Just a couple of string changes here

::: browser/devtools/styleeditor/test/browser_styleeditor_sv_resize.js
@@ +1,5 @@
>  /* vim: set ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +// Test that resizing the fource editor container doesn't move the caret.

Looks like a typo - "fource editor"?

@@ +11,2 @@
>  
> +  is(ui.editors.length, 2, "there is 2 stylesheets initially");

/is/are

@@ +26,2 @@
>  
> +  info("Resising window.");

"Resizing"
Attachment #8592723 - Flags: review?(bgrinstead) → review+
Comment on attachment 8592718 [details] [diff] [review]
bug-1148893-19-autocomplete.patch

Review of attachment 8592718 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!
Attachment #8592718 - Flags: review?(bgrinstead) → review+
Typos addressed.
Attachment #8592723 - Attachment is obsolete: true
Attachment #8594292 - Flags: review+
Following patches are ready to be checked in:
bug-1148893-12-inline-friendly-names.patch
bug-1148893-13-import-rules.patch
bug-1148893-14-import.patch
bug-1148893-15-save.patch
bug-1148893-16-enabled.patch
bug-1148893-17-middle-click.patch
bug-1148893-18-iframes.patch
bug-1148893-19-autocomplete.patch
bug-1148893-20-resize.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb1754de12fa
Keywords: checkin-needed
One more and then I can remove the old stuff from head.js.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fde5ba00ce98
Attachment #8594298 - Flags: review?(bgrinstead)
A patch that removes the old methods from head.js. All other patches need to land and stick before this one can land.
Attachment #8594302 - Flags: review?(bgrinstead)
Attachment #8594298 - Flags: review?(bgrinstead) → review+
Comment on attachment 8594302 [details] [diff] [review]
bug-1148893-22-head.patch

Review of attachment 8594302 [details] [diff] [review]:
-----------------------------------------------------------------

r=me!
Attachment #8594302 - Flags: review?(bgrinstead) → review+
The last patches are ready to land:
bug-1148893-21-opentab.patch
bug-1148893-22-head.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a628b8088f9
Keywords: checkin-needed
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/5975aac16762
https://hg.mozilla.org/mozilla-central/rev/6bac6a4be810
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.