1210936 - Remote New Tab Content Frame and AboutRedirector location

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Olivier Yiptong [:oyiptong]]

This bug is about implementing the remote newtab about page location.

The Remote newtab page is made accessible by navigating to about:remote-newtab.

This loads an xhtml page in a content frame which contains an iframe and a content script to manage communications with the page.

Comment 1

[Olivier Yiptong [:oyiptong]]

Attachment: bug_1210936.patch

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey oyiptong - about 99% of this patch is this newTab.inadjacent.json file that's not referred to by anything... was this supposed to be part of this patch?

Comment 3

[Olivier Yiptong [:oyiptong]]

Ah. I forgot to remove this file. I'll amend the patch

Comment 4

[Olivier Yiptong [:oyiptong]]

Attachment: bug_1210936.v2.patch

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669361 [details] [diff] [review]
bug_1210936.v2.patch

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

Hey oyiptong - this looks pretty good. I just have a few questions - see below.

::: browser/base/content/remote-newtab/newTab.css
@@ +7,5 @@
> +  height: 100%;
> +}
> +
> +body {
> +  font: message-box;

Why does this body need a font? As far as I can tell, there's no written text in newTab.xhtml.

@@ +12,5 @@
> +  width: 100%;
> +  height: 100%;
> +  padding: 0;
> +  margin: 0;
> +  background-color: #F9F9F9;

Why is a background-color needed? The remotedoc should take the entire dimensions of the body, no?

::: browser/base/content/remote-newtab/newTab.js
@@ +10,5 @@
> +    interfaces: Ci
> +  } = Components;
> +  const {
> +    XPCOMUtils
> +  } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});

What's the advantage of doing this over

Cu.import("resource://gre/modules/XPCOMUtils.jsm"); ?

@@ +12,5 @@
> +  const {
> +    XPCOMUtils
> +  } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +  const imports = {};
> +  XPCOMUtils.defineLazyModuleGetter(imports, "PrivateBrowsingUtils",

Instead of attaching the lazy getters to imports, why not this? Then you can just access Services, PrivateBrowsingUtils, from within this function scope.

@@ +32,5 @@
> +    case "NewTab:GetInitialState":
> +      getInitialState();
> +      break;
> +    default:
> +      commandHandled = false;

Why not just return false here?

@@ +34,5 @@
> +      break;
> +    default:
> +      commandHandled = false;
> +    }
> +    return commandHandled;

return true?

@@ +45,5 @@
> +    let remoteIFrame = document.querySelector("#remotedoc");
> +
> +    let loadHandler = () => {
> +      remoteIFrame.removeEventListener("load", loadHandler);
> +      remoteIFrame.contentDocument.addEventListener("NewTabCommand", (e) => {

Before we react to this NewTabCommand, it might be prudent to ensure that is actually coming from the expected URL, and not some hidden subframe or something.

@@ +82,5 @@
> +      windowID: window.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIDOMWindowUtils).outerWindowID,
> +      privateBrowsingMode: isPrivate
> +    };
> +    let remoteIFrame = document.querySelector("#remotedoc");

You do this in 3 places. I guess fetching and memoizing didn't work out?

@@ +89,5 @@
> +      data: state
> +    }, remoteNewTabLocation.origin);
> +  }
> +
> +  addMessageListener("NewTabFrame:Init", function loadHandler(message) {

Who is sending this message?

@@ +94,5 @@
> +    // Everything is loaded. Initialize the New Tab Page.
> +    removeMessageListener("NewTabFrame:Init", loadHandler);
> +    initRemotePage(message.data);
> +  });
> +  sendAsyncMessage("NewTabFrame:GetInit");

Who is receiving this message? Can I assume that sending NewTabFrame:Init after the parent sees RemotePage:Load wasn't sufficient for your purposes?

::: browser/base/jar.mn
@@ +108,5 @@
>          content/browser/newtab/newTab.xhtml           (content/newtab/newTab.xhtml)
>  *       content/browser/newtab/newTab.js              (content/newtab/newTab.js)
>          content/browser/newtab/newTab.css             (content/newtab/newTab.css)
>          content/browser/newtab/newTab.inadjacent.json         (content/newtab/newTab.inadjacent.json)
> +        content/browser/remote-newtab/newTab.xhtml           (content/remote-newtab/newTab.xhtml)

Nit - might as well indent this to match the entries below

Comment 6

[Olivier Yiptong [:oyiptong]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> ::: browser/base/content/remote-newtab/newTab.css
> @@ +7,5 @@
> > +  height: 100%;
> > +}
> > +
> > +body {
> > +  font: message-box;
> 
> Why does this body need a font? As far as I can tell, there's no written
> text in newTab.xhtml.
> 
> @@ +12,5 @@
> > +  width: 100%;
> > +  height: 100%;
> > +  padding: 0;
> > +  margin: 0;
> > +  background-color: #F9F9F9;
> 
> Why is a background-color needed? The remotedoc should take the entire
> dimensions of the body, no?

Thanks. I removed those. They are unnecessary.

> ::: browser/base/content/remote-newtab/newTab.js
> @@ +10,5 @@
> > +    interfaces: Ci
> > +  } = Components;
> > +  const {
> > +    XPCOMUtils
> > +  } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> 
> What's the advantage of doing this over
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm"); ?
> 
> @@ +12,5 @@
> > +  const {
> > +    XPCOMUtils
> > +  } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> > +  const imports = {};
> > +  XPCOMUtils.defineLazyModuleGetter(imports, "PrivateBrowsingUtils",
> 
> Instead of attaching the lazy getters to imports, why not this? Then you can
> just access Services, PrivateBrowsingUtils, from within this function scope.

Changed the invocation in an upcoming patch to do the imports in the global scope.

> @@ +32,5 @@
> > +    case "NewTab:GetInitialState":
> > +      getInitialState();
> > +      break;
> > +    default:
> > +      commandHandled = false;
> 
> Why not just return false here?
> 
> @@ +34,5 @@
> > +      break;
> > +    default:
> > +      commandHandled = false;
> > +    }
> > +    return commandHandled;
> 
> return true?

That's a manifestation of the "single-exit rule". It's a coding convention coming from the C days, carried over today. Many contend that it makes reading switch-case statements easier. I tend to agree.

> @@ +45,5 @@
> > +    let remoteIFrame = document.querySelector("#remotedoc");
> > +
> > +    let loadHandler = () => {
> > +      remoteIFrame.removeEventListener("load", loadHandler);
> > +      remoteIFrame.contentDocument.addEventListener("NewTabCommand", (e) => {
> 
> Before we react to this NewTabCommand, it might be prudent to ensure that is
> actually coming from the expected URL, and not some hidden subframe or
> something.

Good point. It was in the code at some point. Must've been lost when refactoring. Ditto with memoization/caching.

> @@ +89,5 @@
> > +      data: state
> > +    }, remoteNewTabLocation.origin);
> > +  }
> > +
> > +  addMessageListener("NewTabFrame:Init", function loadHandler(message) {
> 
> Who is sending this message?

AboutRemoteNewTab.jsm is sending this message, tracked in bug 1210940.

> @@ +94,5 @@
> > +    // Everything is loaded. Initialize the New Tab Page.
> > +    removeMessageListener("NewTabFrame:Init", loadHandler);
> > +    initRemotePage(message.data);
> > +  });
> > +  sendAsyncMessage("NewTabFrame:GetInit");
> 
> Who is receiving this message? Can I assume that sending NewTabFrame:Init
> after the parent sees RemotePage:Load wasn't sufficient for your purposes?

AboutRemoteNewTab.jsm is receiving this message, tracked in bug 1210940.

RemotePage:Load does not work for us, due to bug 1207283. The workaround has been to send a message from the content frame and expect a response from RemoteAboutNewTab.jsm.

Comment 7

[Olivier Yiptong [:oyiptong]]

Attachment: bug_1210936.v3.patch

Comment 8

[Olivier Yiptong [:oyiptong]]

try server build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cd32163d9e3

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8670062 [details] [diff] [review]
bug_1210936.v3.patch

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

r=me with the -moz-box-flex removed (assuming it should be - see below), and with the requested documentation! Thanks oyiptong!

::: browser/base/content/remote-newtab/newTab.css
@@ +12,5 @@
> +  height: 100%;
> +  padding: 0;
> +  margin: 0;
> +  position: relative;
> +  -moz-box-flex: 1;

Also curious to know if this is necessary. At the very least, we should avoid using the old-school XUL -moz-box-flex stuff, and go with CSS3 flexbox if we need it... but I don't think we do, unless I'm very much mistaken.

Same with the -moz-user-focus and -moz-box-orient... I don't think those are necessary. Am I wrong?

::: browser/base/content/remote-newtab/newTab.js
@@ +15,5 @@
> +(function() {
> +  let remoteNewTabLocation;
> +  let remoteIFrame;
> +
> +  function handleCommand(command, data) {

I'd love some documentation for each of these functions, including what they're expecting as arguments.

Comment 10

[Olivier Yiptong [:oyiptong]]

Attachment: bug_1210936.v4.patch

Comment 11

[Olivier Yiptong [:oyiptong]]

attached the fixed version. Thanks mconley!

Comment 12

[Olivier Yiptong [:oyiptong]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfab6a14cac88a0d0f3c7404ac77d9ba99a74291
Bug 1210936 - Remote New Tab Content Frame and AboutRedirector location r=mconley

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d9fab92d088d for  bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=15465542&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=15465449&repo=mozilla-inbound

Comment 14

[Olivier Yiptong [:oyiptong]]

investigating

Comment 15

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac5027f9d733

Comment 16

[Olivier Yiptong [:oyiptong]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43db293de937fe0300d489acfc9a35f5c6bb19af
Bug 1210936 - Remote New Tab Content Frame and AboutRedirector location r=mconley

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43db293de937