Closed Bug 1287209 Opened 8 years ago Closed 8 years ago

Popup and options UI resizing code is not compatible with remote browsers

Categories

(WebExtensions :: Frontend, defect, P3)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 2 open bugs)

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1287209: Handle popup and options browser resizing using a frame script.
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1287209: Make popup tests compatible with remote browsers.
58 bytes, text/x-review-board-request
aswan
: review+
Details 
Browser action and page action popups, and inline options UI browsers, are automatically sized to fit their content. The current size checking and mutation observer code relies on having access to the content window, which means that it needs to be moved to a frame script, and notify the parent of size changes via the message manager or IPDL.
Whiteboard: triaged
Blocks: webext-popups
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
Assignee: nobody → kmaglione+bmo
Comment on attachment 8802376 [details]
Bug 1287209: Handle popup and options browser resizing using a frame script.

https://reviewboard.mozilla.org/r/86788/#review85972

Mostly nits.
Meta-comment: for a reasonbly complex patch like this (complex to somebody like me who doesn't already know the underlying code as well as you do anyway), unrelated refactorings and cleanups make reading and understanding the patch harder than it needs to be, please put those in separate commits.

::: browser/components/extensions/ext-utils.js:60
(Diff revision 2)
>        });
>      }
>    });
>  }
>  
>  XPCOMUtils.defineLazyGetter(this, "stylesheets", () => {

nit: now that this isn't actually doing anything very expensive, the lazy getter seems like overkill

::: browser/components/extensions/ext-utils.js:146
(Diff revision 2)
>      });
>    }
>  
>    destroyBrowser(browser) {
> -    browser.removeEventListener("DOMWindowCreated", this, true);
> -    browser.removeEventListener("load", this, true);
> +    let mm = browser.messageManager;
> +    if (mm) {

under what circumstances do we not have an mm here?

::: browser/components/extensions/ext-utils.js:300
(Diff revision 2)
> +    }
> +
> +    let event = new this.window.CustomEvent("WebExtPopupResized", {detail});
> +    this.browser.dispatchEvent(event);
> +
> +    this._resolveContentReady();

you're also doing this immediately when you get the resize event (line 197), can this one go?

::: toolkit/components/extensions/ExtensionUtils.jsm:1141
(Diff revision 2)
>      });
>    });
>  }
>  
>  /**
> + * Returns a Promise which resolves the given event is dispatched to the

nit: ...resolves *when* the given event...

::: toolkit/components/extensions/ExtensionUtils.jsm:1146
(Diff revision 2)
> + *        observer's subject and data, should return true if this is the
> + *        expected event, false otherwise.

looks like stray lines left over from a bungled copy/paste maybe?
Attachment #8802376 - Flags: review?(aswan) → review+
Comment on attachment 8802376 [details]
Bug 1287209: Handle popup and options browser resizing using a frame script.

https://reviewboard.mozilla.org/r/86788/#review85972

> under what circumstances do we not have an mm here?

When the browser has already been removed from the document because the popup was closed externally.

> you're also doing this immediately when you get the resize event (line 197), can this one go?

Yeah.

> looks like stray lines left over from a bungled copy/paste maybe?

Yeah. No idea how that happened.
Comment on attachment 8802754 [details]
Bug 1287209: Make popup tests compatible with remote browsers.

https://reviewboard.mozilla.org/r/87052/#review86096

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:120
(Diff revision 1)
>  
>    yield extension.startup();
>  
> +  /* eslint-disable mozilla/no-cpows-in-tests */
> +
> +  function alter(browser, task) {

you do a bunch of stuff that follows (roughly) this same pattern in the next test as well, maybe move this to head.js?

::: browser/components/extensions/test/browser/head.js:113
(Diff revision 1)
> +  // Debouncing code makes this a bit racy.
> +  // Try to skip the first, early resize, and catch the resize event we're
> +  // looking for, but don't wait longer than a few seconds.

I don't understand the issue here?  With the current debouncing implementation, I think you should always see the delayed event unless the window goes away but you wouldnt be using this on a window that can disappear... ?
Attachment #8802754 - Flags: review?(aswan) → review+
Comment on attachment 8802754 [details]
Bug 1287209: Make popup tests compatible with remote browsers.

https://reviewboard.mozilla.org/r/87052/#review86096

> I don't understand the issue here?  With the current debouncing implementation, I think you should always see the delayed event unless the window goes away but you wouldnt be using this on a window that can disappear... ?

Oh, yeah, this comment isn't relevant anymore. It applied to the old implementation.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68806639c03110bfb67dd6c705a5441a917afcd6
Bug 1287209: Handle popup and options browser resizing using a frame script. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/642ee7eb70bbe1d512a06cf6478910bd2d0db273
Bug 1287209: Make popup tests compatible with remote browsers. r=aswan
Backed out for failing browser-chrome test browser_ext_popup_corners.js:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=642ee7eb70bbe1d512a06cf6478910bd2d0db273
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37960294&repo=mozilla-inbound

[task 2016-10-20T05:55:35.757362Z] 05:55:35     INFO -  425 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderBottomRightRadius should be the same -
[task 2016-10-20T05:55:35.759704Z] 05:55:35     INFO -  426 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderBottomLeftRadius should be the same -
[task 2016-10-20T05:55:35.766000Z] 05:55:35     INFO -  427 INFO TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderBottomLeftRadius should be the same -
[task 2016-10-20T05:55:35.767867Z] 05:55:35     INFO -  428 INFO Test menu panel browserAction popup
[task 2016-10-20T05:55:35.770642Z] 05:55:35     INFO -  429 INFO Console message: [JavaScript Error: "Error: Popup destroyed" {file: "chrome://browser/content/ext-utils.js" line: 124}]
[task 2016-10-20T05:55:35.772532Z] 05:55:35     INFO -  430 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Uncaught exception - TypeError: Argument 1 of Window.getComputedStyle is not an object.
[task 2016-10-20T05:55:35.774303Z] 05:55:35     INFO -  431 INFO Leaving test bound testPopupBorderRadius
[task 2016-10-20T05:55:35.776693Z] 05:55:35     INFO -  Not taking screenshot here: see the one that was previously logged
[task 2016-10-20T05:55:35.778614Z] 05:55:35     INFO -  432 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Extension left running at test shutdown -
[task 2016-10-20T05:55:35.783899Z] 05:55:35     INFO -  Stack trace:
[task 2016-10-20T05:55:35.785679Z] 05:55:35     INFO -      chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:107
[task 2016-10-20T05:55:35.787667Z] 05:55:35     INFO -      chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:435
[task 2016-10-20T05:55:35.789425Z] 05:55:35     INFO -      testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1004:11
[task 2016-10-20T05:55:35.791121Z] 05:55:35     INFO -      testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:935:9
Flags: needinfo?(kmaglione+bmo)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e964a086a0
Backed out changeset 642ee7eb70bb 
https://hg.mozilla.org/integration/mozilla-inbound/rev/787c5e10eae7
Backed out changeset 68806639c031 for failing browser-chrome test browser_ext_popup_corners.js. r=backout
I hate that test...
Flags: needinfo?(kmaglione+bmo)
Wups, no, browser_ext_popup_corners.js is coming from somewhere below this.
Oh, no it's not, it's coming from the previous time this landed.
Keeping fingers crossed... So far clean try runs seem to have no correlation with what happens on inbound...
Depends on: 1312224
https://hg.mozilla.org/mozilla-central/rev/442e28121564
https://hg.mozilla.org/mozilla-central/rev/18a51ecab347
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1463012
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: