Closed
Bug 1054600
Opened 10 years ago
Closed 10 years ago
Refactor AboutHomeListener & browser.js document URI checks (followup from bug 1041678)
Categories
(Firefox :: Search, defect)
Tracking
()
People
(Reporter: alexbardas, Assigned: alexbardas)
References
Details
Attachments
(1 file, 3 obsolete files)
7.28 KB,
patch
|
alexbardas
:
review+
|
Details | Diff | Splinter Review |
AboutHomeListener.onUpdate checks the document URI. onFocusInput should, too. That check should be factored out into its own method, and then both receiveMessage and handleEvent should call it so that all the various methods that they delegate to don't have to, and we don't get tripped up by this again.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8474046 -
Flags: review?(adw)
Comment 2•10 years ago
|
||
Comment on attachment 8474046 [details] [diff] [review] Refactor documentURI checks in AboutHomeListener and browser.js Patch #1 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > let openSearchPageIfFieldIsNotActive = function(aSearchBar) { >- let doc = gBrowser.selectedBrowser.contentDocument; >- let url = doc.documentURI.toLowerCase(); >+ let url = gBrowser.currentURI.spec; You probably want to keep the toLowerCase()
Updated•10 years ago
|
QA Whiteboard: [qa?]
Flags: firefox-backlog+
Assignee | ||
Comment 3•10 years ago
|
||
Keep .toLowerCase() logic
Attachment #8474046 -
Attachment is obsolete: true
Attachment #8474046 -
Flags: review?(adw)
Attachment #8474184 -
Flags: review?(adw)
Comment 4•10 years ago
|
||
Comment on attachment 8474184 [details] [diff] [review] Refactor documentURI checks in AboutHomeListener and browser.js Patch #2 >--- a/browser/base/content/content.js >+++ b/browser/base/content/content.js >+// Get the current document uri. >+function getURI() { >+ return content.document.documentURI.toLowerCase(); >+} While you might not care about upper/lower case differences in about: URIs, other consumer might care for other URIs. Introducing what looks like a general-purpose utility function that silently makes all URIs lower case doesn't seem sane.
Comment 5•10 years ago
|
||
Comment on attachment 8474184 [details] [diff] [review] Refactor documentURI checks in AboutHomeListener and browser.js Patch #2 Review of attachment 8474184 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +24,5 @@ > return Services.io.newURI(uri, originCharset, baseURI); > } > > +// Get the current document uri. > +function getURI() { I agree with Dao about this. What I meant by factoring out is the uri == "about:home" check in AboutHomeListener, so there should be something like an AboutHomeListener.isAboutHome getter that AboutHomeListener calls internally, similar to the ContentSearchMediator._contentWhitelisted getter. @@ +33,5 @@ > // Hide session restore button on about:home > let doc = content.document; > let container; > + if (getURI() == "about:home" && > + (container = doc.getElementById("sessionRestoreContainer"))) { So here, it's fine to leave this as it is, no refactoring necessary. Or, this could call AboutHomeListener.isAboutHome. @@ +97,5 @@ > init: function(chromeGlobal) { > chromeGlobal.addEventListener('AboutHomeLoad', () => this.onPageLoad(), false, true); > }, > > handleEvent: function(aEvent) { I didn't realize this earlier, but this method is never actually called because nobody adds an event listener with AboutHomeListener as the target. It looks like a mistake because that's always been the case since bug 899222 landed the AboutHomeListener (http://hg.mozilla.org/mozilla-central/rev/0f68b32be429). I think we should change all the addEventListener calls to pass `this` (AboutHomeListener) as the target so that all events are routed through here. Then we can do the uri == "about:home" check here. That means we need to move the pagehide and AboutHomeSearchEvent listener functions out of onPageLoad into their own top-level methods. Does anybody disagree?
Attachment #8474184 -
Flags: review?(adw)
Updated•10 years ago
|
QA Whiteboard: [qa?] → [qa-]
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8474184 -
Attachment is obsolete: true
Attachment #8474701 -
Flags: review?(adw)
Comment 7•10 years ago
|
||
Comment on attachment 8474701 [details] [diff] [review] Refactor documentURI checks in AboutHomeListener and browser.js Patch #3 Review of attachment 8474701 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r+ with the changes below. ::: browser/base/content/content.js @@ +89,5 @@ > } > > let AboutHomeListener = { > init: function(chromeGlobal) { > chromeGlobal.addEventListener('AboutHomeLoad', () => this.onPageLoad(), false, true); Need to make the target `this` instead of the anonymous function so that handleEvent is called. @@ +110,5 @@ > + break; > + case "pagehide": > + this.onPageHide(aEvent); > + break; > + case "AboutHomeSearchEvent": Nit: please move this above the "click" case so that the cases are in alphabetical order. @@ +148,5 @@ > }, > > onPageLoad: function() { > let doc = content.document; > + if (!this.isAboutHome || doc.documentElement.hasAttribute("hasBrowserHandlers")) { The !this.isAboutHome term can be removed with the `this` target change I mentioned above. @@ +239,3 @@ > onFocusInput: function () { > + let searchInput = content.document.getElementById("searchText"); > + if (searchInput) { Now that we're properly ensuring that the page is about:home before calling this method, this conditional shouldn't be necessary since the input should always be in the page, but I guess it couldn't hurt to check, so I won't ask you to remove it.
Attachment #8474701 -
Flags: review?(adw) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8474701 -
Attachment is obsolete: true
Attachment #8474741 -
Flags: review+
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b9d3bfad08d8 https://hg.mozilla.org/integration/fx-team/rev/976ddf70355a
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/976ddf70355a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Iteration: --- → 34.3
Points: --- → 2
QA Whiteboard: [qa-]
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•