511020 - Don't call onLocationChange or onSecurityChange (since they didn't) when we switch tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Stuart Parmenter]

We shouldn't be calling event listener functions directly

Comment 1

[Stuart Parmenter]

Attachment: v0.8

Comment 2

[Stuart Parmenter]

Attachment: v0.9

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 394971 [details] [diff] [review]
v0.9

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+      let selectedBrowser = tab.browser;
>+      let webProgress = selectedBrowser.webProgress;

Do we need this?

>+	// XXX this is gross.
>+	let currentLocation = tab.browser.contentWindow.location;
>+	getIdentityHandler().checkIdentity(tab._listener._state, currentLocation);

Use spaces instead of tabs to indent here

>+    function makeLocationObject(location) {
>+      try {
>+	return {'host' : location.host,
>+	    'hostname' : location.hostname,
>+	    'port' : location.port };

Use " instead of '

> function ProgressController(tab) {
>   this._tab = tab;
>+ // Properties used to cache security state used to update the UI
>+  this._state = null;

1 more space indent on the comment

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 394971 [details] [diff] [review]
v0.9

I don't really see the point of this patch... It makes it harder to be sure that tab switching location changing are in sync in terms of what they update in the UI, and makes the onSecurityChange code diverge from the Firefox code for little gain.

I agree that we should clean up onLocationChange, but I don't think any of the other changes are necessary.

(That patch also doesn't remove lastURI completely, as mentioned on IRC, and introduces the unused variables in the selectedTab setter.)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> >+	    'port' : location.port };
> 
> Use " instead of '

Or just omit the quotes :)

Comment 6

[Benjamin Stover (:stechz)]

Attachment: v0.9.ben

Addresses code review issues.  Refactored security update stuff to one location.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: v0.9.1.ben

Bugfix

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 395953 [details] [diff] [review]
v0.9.1.ben

>+      // Update all of our UI to reflect the new tab's location
>+      BrowserUI.setURI();
>+      tab._updateSecurity();

If you're calling it as a public method, don't use the "_" prefix

>+    function makeLocationObject(location) {
>+      try {
>+	return { host: location.host, hostname: location.hostname, port: location.port };

Someone else like tabs for indenting! Call dougt! :)

> function ProgressController(tab) {
>   this._tab = tab;
>+  // Properties used to cache security state used to update the UI

Add a blank line above the comment (space to breathe)

>+  _updateSecurity: function() {
>+    this.ensureBrowserExists();
>+    let securityUI = this.browser.securityUI;

Maybe we should put "ensureBrowserExists()" inside the Tab.browser getter and add a Tab.hasBrowser property for those times you want to check.

I'd r+, but I want us to consider the ensureBrowserExists issue

Comment 9

[:Gavin Sharp [email: [e-mail]]]

That patch still doesn't remove the lastURI completely (it's only use is in the code that's being removed).

It also makes us re-get the location object on each call to onSecurityChange, which will likely hurt Tp (and _host is now unused).

Comment 10

[ben]

Agreed on formatting issues (did not realize I was using tabs :/).

> If you're calling it as a public method, don't use the "_" prefix

I see it as a private method.  It seemed that everything in browser.js was already tightly coupled, so I didn't feel bad touching private things in browser.js while inside browser.js.

> Maybe we should put "ensureBrowserExists()" inside the Tab.browser getter and
> add a Tab.hasBrowser property for those times you want to check.

That smells like a dangerous side effect of a getter.  I'd want people to *know* that they are creating content inside a tab (and that it wasn't there before) with a call to ensureBrowserExists instead of accidentally accessing browser, not realizing what they'd done.

> It also makes us re-get the location object on each call to onSecurityChange,
> which will likely hurt Tp (and _host is now unused).

Is getting location that bad of a hit?  onSecurityChange should only access it if the UI needs to be updated, which happens on a load or on a tab change (I think; please correct if I'm wrong).

Comment 11

[:Gavin Sharp [email: [e-mail]]]

The problem was that onSecurityChange is (was?) called more often than it needs to be. See bug 398360 and bug 397492 for some context. Bug 451420 was filed on PSM and has since been fixed, so we should probably revisit whether the benefit to reducing gets to .location is worth the code complexity cost, but I'd rather stick with the status quo until we measure that.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Turns out I was wrong, and _host isn't actually used! It was never used in Fennec, and stopped being used in http://hg.mozilla.org/mozilla-central/rev/2acc8c5989ae in Firefox. Feel free to ignore that comment.

Comment 13

[Benjamin Stover (:stechz)]

Attachment: v0.9.2.ben

Fixed formatting issues and removed some unused variables (thanks Gavin).

Comment 14

[Benjamin Stover (:stechz)]

Attachment: Patch now works with trunk

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 397310 [details] [diff] [review]
Patch now works with trunk

We talked on IRC about removing Tab._updateSecurity and making IdentityHandler.checkSecurity handle the work itself.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 397310 [details] [diff] [review]
Patch now works with trunk

I didn't see mfinkle's comment before starting this, so maybe these comments are made irrelevant by the newly proposed changes you were discussing...

>diff -r 54c827234898 -r acee0c0a176f chrome/content/browser.js

>   // This method is called to indicate a change to the current location.
>   onLocationChange: function(aWebProgress, aRequest, aLocationURI) {

>-    //don't do anything for about:blank or about:firstrun on first display
>-    if (!lastURI && (location == "about:blank" || location == "about:firstrun" ))
>-      return;

We'll need to watch and see whether this has any Ts impact... Would be good to actually measure this.

>+  // This method is called when the security state of the browser changes.
>+  onSecurityChange: function(aWebProgress, aRequest, aState) {

>+    if (aWebProgress.DOMWindow == Browser.selectedBrowser.contentWindow) {
>+      this._tab._updateSecurity();

I think what we should do here instead is keep a back reference to the tab object in the listener, and compare it against selectedTab. I think the checks are equivalent (looks like DOMWindow can only point to the top-level window that corresponds to the docshell the listener was added to), but this check should be slightly faster, and it's less ambiguous. Same applies to onLocationChange.

We could take this a step further and avoid doing work in _updateSecurity called from |set selectedTab| if the hostname and state haven't changed. This is how the code works in Firefox - the listeners only deal with events from the active tab, so the caching of state/location is global rather than per-tab. I was confused for a bit because I didn't realize that we were dealing with per-tab listeners here.

Comment 17

[Benjamin Stover (:stechz)]

Attachment: Remove updateSecurity

Changes talked about in IRC

Comment 18

[Benjamin Stover (:stechz)]

Attachment: Code review and & bugfix

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 399293 [details] [diff] [review]
Code review and & bugfix

looks good to me

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Although, I agree with gavin that we need to watch Ts

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 399293 [details] [diff] [review]
Code review and & bugfix

>diff -r 0c5a89a8d1f3 -r 023444c66187 chrome/content/browser.js

>+  checkIdentity: function() {
>+    // make a copy of the passed in location to avoid cycles
>+    function makeLocationObject() {
>+      try {
>+        return { host: location.host, hostname: location.hostname, port: location.port };
>+      } catch (ex) { }
>+      return {};
>+    }

>+    this._lastLocation = makeLocationObject(location);

just a nit: a helper for this seems a bit heavy for something only called once - just inline it?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Made gavin's tweak and pushed:
https://hg.mozilla.org/mobile-browser/rev/dbb31bdf7d4f

Comment 23

[Aakash Desai [:aakashd]]

How would I verify this?

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #23)
> How would I verify this?

2 bugs were fixed by this I think:
* SSL indicator is incorrect
** open a new tab
** go to addons.mozilla.org, indicator is green
** go to firstrun tab, indicator stayed green
* Page scrolled to top when switching tabs
** open a new tab
** go to planet.m.o and scroll down
** go to firstrun tab and then back to planet tab, page was scrolled to top

Those two bugs should be fixed now

Comment 25

[Aakash Desai [:aakashd]]

..and they were verified as well.