1461444 - Only load the PageInfoListener content code when needed

Status: RESOLVED FIXED

(Firefox :: Page Info Window, enhancement)

Description

[:Felipe Gomes (needinfo for replies!)]

An instance of the PageInfoListener object is created for every tab (in content.js) only to add the message listener for PageInfo:getData

We can add this listener in content.js and only load the .jsm when that happens.

This patch uses the helper being added in bug 1457988

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1461444 - Part 0. Cut and paste PageInfoListener code from content.js to a new file.

This is to simplify the review process and highlight the differences in a separate patch. The code in the new file is an exact copy of the old one.

Review commit: https://reviewboard.mozilla.org/r/243856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243856/

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1461444 - Only load the PageInfoListener content code when needed.

In addition to the code going to a .jsm, it became a singleton that implements nsIMessageListener, instead of being one instance per tab (which is usually done to give easier access to the page's content, but it's possible to get to it through each message, as message.target.content

Review commit: https://reviewboard.mozilla.org/r/243858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243858/

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8975605 [details]
Bug 1461444 - Part 0. Cut and paste PageInfoListener code from content.js to a new file.

https://reviewboard.mozilla.org/r/243856/#review249946

I'm not really sure about the name, but the patch looks fine otherwise :)

::: browser/modules/PageInfoListener-content.jsm:1
(Diff revision 1)
> +var PageInfoListener = {

Aren't we usually calling these scripts something like ContentPageInfoListener? I haven't seen the CamelCaseName-content pattern before...

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8975606 [details]
Bug 1461444 - Only load the PageInfoListener content code when needed.

https://reviewboard.mozilla.org/r/243858/#review249968

This seems good, thanks!

::: browser/modules/PageInfoListener-content.jsm:26
(Diff revision 1)
>      if (frameOuterWindowID != undefined) {
>        window = Services.wm.getOuterWindowWithId(frameOuterWindowID);
>        document = window.document;
>      } else {
> -      window = content.window;
> -      document = content.document;
> +      window = message.target.content;
> +      document = window.document;

nit: having this in an if-else block is a bit redundant now

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fba5430ad51
Part 0. Copy PageInfoListener code from content.js to a new file. r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7733393d2e3
Only load the PageInfoListener content code when needed. r=johannh

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6fba5430ad51
https://hg.mozilla.org/mozilla-central/rev/b7733393d2e3