Closed
Bug 1210936
Opened 9 years ago
Closed 9 years ago
Remote New Tab Content Frame and AboutRedirector location
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: oyiptong, Assigned: oyiptong)
References
Details
Attachments
(1 file, 3 obsolete files)
15.04 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8669113 -
Flags: review?(mconley)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → oyiptong
Comment 2•9 years ago
|
||
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?
Flags: needinfo?(oyiptong)
Assignee | ||
Comment 3•9 years ago
|
||
Ah. I forgot to remove this file. I'll amend the patch
Flags: needinfo?(oyiptong)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8669113 -
Attachment is obsolete: true
Attachment #8669113 -
Flags: review?(mconley)
Assignee | ||
Updated•9 years ago
|
Attachment #8669361 -
Flags: review?(mconley)
Comment 5•9 years ago
|
||
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
Attachment #8669361 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8669361 -
Attachment is obsolete: true
Attachment #8670062 -
Flags: review?(mconley)
Assignee | ||
Comment 8•9 years ago
|
||
try server build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cd32163d9e3
Comment 9•9 years ago
|
||
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.
Attachment #8670062 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8670062 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
attached the fixed version. Thanks mconley!
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfab6a14cac88a0d0f3c7404ac77d9ba99a74291 Bug 1210936 - Remote New Tab Content Frame and AboutRedirector location r=mconley
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
Flags: needinfo?(oyiptong)
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac5027f9d733
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43db293de937fe0300d489acfc9a35f5c6bb19af Bug 1210936 - Remote New Tab Content Frame and AboutRedirector location r=mconley
Comment 17•9 years ago
|
||
bugherder merge |
https://hg.mozilla.org/mozilla-central/rev/43db293de937
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in
before you can comment on or make changes to this bug.
Description
•