729638 - Cleanup/Refactor Fullscreen code

Status: NEW

(Firefox :: General, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

It's a bit messy and sometimes redundant. It could use a good cleanup & maybe be moved into it's own file & #included.

I'm not going to work on this just yet, so if somebody want's to come along and pick up a first bug that is less trivial than changing a couple lines I'd be happy to help that along.

Comment 1

[Josh Matthews [:jdm]]

Want to give some links to the relevant files?

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

The FullScreen object in browser.js is where this all lives: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3939

Comment 3

[Sam Garrett]

I'd be up for giving this a go as a Mentored Bug. This will be my first bug fix. I'm getting the mozilla source code setup and built on my box right now.

Comment 4

[Sam Garrett]

When I put this into it's own file where should I include it?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Sam Garrett from comment #4)
> When I put this into it's own file where should I include it?

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#203

However, I'm not sure that's necessary, and most importantly it shouldn't be done in the same patch that does the refactoring, as this would make the patch impractical for reviewing the changes.

Comment 6

[Sam Garrett]

Good to know. I'll just worry about refactoring this for now then.

Comment 7

[Sam Garrett]

Had some time to look over the code tonight. What redundancies are you seeing? Is there an architecture for this object that you're looking for?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Sam said he'd like to work on this.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Hey Sam, thanks for looking into this. One of the things is that mouseoverToggle isn't an entirely appropriate name (that my have been it's original purpose, but more often than not, it's not called in response to a mouseover. It's also called redundantly during domfullscreen. showXULChrome is _really_ showToolbars.

It's really not that bad and the code there is pretty well documented \o/, but it could use some love.

I noticed it while working on bug 639705, which is close to landing, so you may want to do your work with that patch applied (the browser.js stuff alone should be perfectly fine to have in there).

Comment 10

[Sam Garrett]

Paul,

Regarding showXULChrome, the first parameter "aTag" in showXULChrome: function(aTag, aShow) doesn't look like it even needs to be passed. showXULChrome is only used in one spot and "toolbar" is passed for that parameter. Would you like me to remove that parameter? Or is showXULChrome used somewhere else in the application?

Also are there a set of regression tests that I can run when I make my changes? And do I need to run make for the whole project again once I've made the changes?

Comment 11

[Dão Gottwald [:dao]]

(In reply to Sam Garrett from comment #10)
> Paul,
> 
> Regarding showXULChrome, the first parameter "aTag" in showXULChrome:
> function(aTag, aShow) doesn't look like it even needs to be passed.
> showXULChrome is only used in one spot and "toolbar" is passed for that
> parameter. Would you like me to remove that parameter?

Sounds like it should be removed.

> Or is showXULChrome used somewhere else in the application?

Nope, see http://mxr.mozilla.org/mozilla-central/search?string=showxulchrome

> Also are there a set of regression tests that I can run when I make my
> changes?

I'm afraid there aren't any useful tests for this. I'm not even sure we have browser chrome tests entering full-screen mode at all.

> And do I need to run make for the whole project again once I've
> made the changes?

You can run make in $objdir/browser/base.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Dão Gottwald [:dao] from comment #11)
> > Also are there a set of regression tests that I can run when I make my
> > changes?
> 
> I'm afraid there aren't any useful tests for this. I'm not even sure we have
> browser chrome tests entering full-screen mode at all.

Yea, I don't know of any. The best I'd found are the dom fullscreen tests, but that's going to cover the normal fullscreen case.

If you want to write some as part of this... :)

> > And do I need to run make for the whole project again once I've
> > made the changes?
> 
> You can run make in $objdir/browser/base.

If you're on a mac, you'll need to build $objdir/browser/app too (though honestly, I just build $objdir/browser most of the time)

Comment 13

[Sam Garrett]

I'd be happy to write some tests for part of this, although I'm not familiar with how they are written.

Comment 14

[Sam Garrett]

Attachment: Cleaning up the FullScreen code.

Please let me know what you think about the approach I'm taking on this. Most of my changes are regarding the event handlers. There were a lot of duplicate calls being made (add/removeEventListener).

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 604543 [details] [diff] [review]
Cleaning up the FullScreen code.

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

I think it's going down a good path. I just bitrotted you by landing bug 639705, which touches the fullscreen code here. In addition to that, I think there are a few more things that can be done - take a look at comment #9 again for some ideas. Other things that I don't think were mentioned: attributes are defined throughout the FullScreen object - we should move them up top as much as possible. As a newcomer to the code, you can probably also spot any areas where the code wasn't super clear or there wasn't enough documentation - adding docs would be awesome too :)

As for tests... if you're up for adding those too, putting a browser_fullscreen.js file in browser/base/content/test (and adding to the Makefile) would be the right way to go about that. Those test are browser-chrome tests - https://developer.mozilla.org/en/Browser_chrome_tests is a good starting point, as well as the other browser_* files in that directory. Let me know when you're ready for that and I can help you a bit more hands-on.

::: browser/base/content/browser.js
@@ +4038,5 @@
>      this.handleFSToggle(false);
>  
> +    this.addOrRemoveScrToggler(false);
> +  },
> +  _addOrRemoveScrToggler: function (toAdd)

I'm not a big fan of this function name. Scr is short for Screen, which doesn't completely make sense without Full in front of it.

Also, make sure you document what's happening.

And nit: Put the opening brace on the same line (I know style is a bit mixed)

@@ -4090,5 @@
>    observe: function(aSubject, aTopic, aData)
>    {
>      if (aData == "browser.fullscreen.autohide") {
> -      gBrowser.mPanelContainer.removeEventListener("mousemove",
> -                                                   this._collapseCallback, false);

Is this safe to get rid of? (ditto in toggle)

Comment 16

[Sam Garrett]

Attachment: Cleaning up the FullScreen code.

I've made quite a few more changes. Let me know if you think this is better.

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 608504 [details] [diff] [review]
Cleaning up the FullScreen code.

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

A few comments since this got to be larger that I'd initially thought (and so it's a little less [good first bug] appropriate but you're doing well!)

1. This still doesn't have the Lion FS stuff in there (but I think you knew that). Bug 737792 is also going to bitrot you.
2. Can we split this across multiple patches? Mixing style cleanup with moving code gets a bit harder to follow. So if part 1 can be style fixes, and then part 2 moving/rearranging code (some of which was just changed for style), that would be really helpful for me.
2. Let's lay down some general style rules
 a. functions inside the FS object should be in this format:
> name: function(param, param...) {
 b. have an empty line between functions
 c. no tabs!

f+ but there's a bit to do - I'll probably have more comments next round.

::: browser/base/content/browser.js
@@ +4048,5 @@
> +
> +    this.addOrRemoveScrToggler(false);
> +  },
> +  // @param toAdd - boolean - whether the EventListener should be added or removed
> +  _addOrRemoveScreenToggler: function (toAdd) {

_addOrRemoveToggler

@@ +4097,5 @@
>        window.removeEventListener("deactivate", this.exitDomFullScreen, true);
>      }
>    },
>  
> +  observe: function (aSubject, aTopic, aData){

Nit: space between ) & {

@@ +4195,5 @@
>      gNavToolbox.style.marginTop = (gNavToolbox.boxObject.height * pos * -1) + "px";
>      this._animationHandle = window.mozRequestAnimationFrame(this);
>    },
>  
> +  _cancelAnimation: function () {

(also applies to other changes you made...)
> _cancelAnimation: function() {

@@ +4281,5 @@
>          3000);
>    },
>  
> +  // @param showToolbar - boolean - whether the toolbar should be shown 
> +  handleFSToggle: function (showToolbar) {

I don't think this name is too much better, especially since we already have a method named toggle.

@@ +4339,5 @@
> +    if (!this._animationHandle) {
> +      this._animationHandle = window.mozRequestAnimationFrame(this);
> +    }
> +  },
> +  // @param - aShow - boolean - whether the toolbar should be shown

We're not consistent about any javadoc style. Just a sentence explaining the function is sufficient. Also, please have whitespace between the end of one function and the next.

@@ +4343,5 @@
> +  // @param - aShow - boolean - whether the toolbar should be shown
> +  _toggleToolbar: function (aShow) {
> +    var els = document.getElementsByTagNameNS(this._XULNS, "toolbar");
> +    var elementIndex = 0;
> +    for (elementIndex; elementIndex < els.length; ++elementIndex) {

This whole loop is actually getting cleaned up in bug 737792, so you're going to get bitrotted again :/ That's ok, I was going to ask you to revert this anyway.

@@ +4434,5 @@
>      var controls = document.getElementsByAttribute("fullscreencontrol", "true");
> +    var controlIndex = 0;
> +    for (controlIndex; controlIndex < controls.length; ++controlIndex) {
> +      controls[controlIndex].hidden = aShow;
> +	}

Nit: this is a tab character. This happened a few other times (looks like when you're adding closing braces) so please clean those out too.

If this hasn't already changed to a for...of loop, we can do that instead of the change you made.

Comment 18

[Sam Garrett]

Attachment: The styling patch for this bug.

This is the first of what I think will be 3 patches:
1) style/formatting and renaming function names patch
2) rearranging code patch
3) refactoring code patch

Comment 19

[Sam Garrett]

Attachment: Rearranging variable properties to top of FullScreen definition.

Comment 20

[Sam Garrett]

Attachment: Refactoring FullScreen.

This refactor is mostly changing the event listeners. Please review this patch along with the other two that I've created.

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 611695 [details] [diff] [review]
The styling patch for this bug.

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

::: browser/base/content/browser.js
@@ +4111,5 @@
> +  _collapseCallback: function() {
> +    FullScreen.toggleUI(false);
> +  },
> +
> +  _keyToggleCallback: function(event) {

don't change aEvent to event.

@@ +4148,5 @@
>      }
>      return true;
>    },
>  
> +  _setPopupOpen: function(event) {

Don't change aEvent to event.

@@ +4177,5 @@
>    _shouldAnimate: true,
>    _isAnimating: false,
>    _animationTimeout: 0,
>    _animationHandle: 0,
> +

no need to add this line here if you're just moving these properties around next

@@ +4342,5 @@
>      if (gPrefService.getIntPref("browser.fullscreen.animateUp") == 2)
>        this._shouldAnimate = true;
>    },
>  
> +  _showToolbar: function(aTag, aShow) {

If it's called showToolbar, we shouldn't need to pass aTag in anymore since it's always "toolbar".

And let's be consistent and called it toggleToolbar

Comment 22

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Paul O'Shannessy [:zpao] from comment #21)
> If it's called showToolbar, we shouldn't need to pass aTag in anymore since
> it's always "toolbar".

Oh I see you did that in part 3. The separation of part 1 & 3 isn't super clear cut. I think doing it all in part 3 would make the most sense since it's not really a style thing at that point.

Comment 23

[Sam Garrett]

(In reply to Paul O'Shannessy [:zpao] from comment #21)
> Comment on attachment 611695 [details] [diff] [review]
> The styling patch for this bug.
> 
> Review of attachment 611695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +4111,5 @@
> > +  _collapseCallback: function() {
> > +    FullScreen.toggleUI(false);
> > +  },
> > +
> > +  _keyToggleCallback: function(event) {
> 
> don't change aEvent to event.
> 
> @@ +4148,5 @@
> >      }
> >      return true;
> >    },
> >  
> > +  _setPopupOpen: function(event) {
> 
> Don't change aEvent to event.

I was trying to go for consistency here. In some places aEvent is passed and in others event is passed. So I just wanted to make them all the same.

> 
> @@ +4177,5 @@
> >    _shouldAnimate: true,
> >    _isAnimating: false,
> >    _animationTimeout: 0,
> >    _animationHandle: 0,
> > +
> 
> no need to add this line here if you're just moving these properties around
> next

We had discussed moving all of the variable properties to the top, but I can remove this in the next patch if you'd like.

I'll get 1 & 3 combined on Friday. I'm out today and tomorrow so I'll be in touch then.

Comment 24

[Sam Garrett]

Attachment: Style patch.

Comment 25

[Sam Garrett]

Attachment: Rearranging FullScreen code.

Comment 26

[Sam Garrett]

Attachment: Refactoring FullScreen.

Comment 27

[Matthias Versen [:Matti]]

Sam: You should switch to a different reviewer. I don't think that Paul still works on Firefox

Comment 28

[Sam Garrett]

(In reply to Matthias Versen [:Matti] from comment #27)
> Sam: You should switch to a different reviewer. I don't think that Paul
> still works on Firefox

@Matthias - I'm not privy to who would be a good reviewer for this anymore as the code is probably sorely outdated now. If these changes are still valid then I'd be happy to update them.

Comment 29

[Matthias Versen [:Matti]]

I'm sorry that this happened. I saw this bug only while searching for other reports.
maybe Dao can suggest what we should do with this patch

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 616355 [details] [diff] [review]
Refactoring FullScreen.

From a quick skim, I don't see any changes that are particularly eyebrow-raising. So I'd suggest going ahead with updating this patch to work on current mozilla-central code. Sorry we didn't catch this much much earlier. :(


One quick comment:

>-    gBrowser.tabContainer.addEventListener("TabOpen", this.exitDomFullScreen);
>-    gBrowser.tabContainer.addEventListener("TabClose", this.exitDomFullScreen);
>-    gBrowser.tabContainer.addEventListener("TabSelect", this.exitDomFullScreen);
>+    this._addOrRemoveTabListeners(true);
...
>+  _addOrRemoveTabListeners: function (toAdd) {
>+    let methodCall = toAdd ? "addEventListener" : "removeEventListener";
>+    // Exit DOM full-screen mode upon open, close, or change tab.
>+    gBrowser.tabContainer[methodCall]("TabOpen", this.exitDomFullScreen);
>+    gBrowser.tabContainer[methodCall]("TabClose", this.exitDomFullScreen);
>+    gBrowser.tabContainer[methodCall]("TabSelect", this.exitDomFullScreen);
>+  },

As a general rule, this is a pattern to avoid. someMethod(true) vs someMethod(false) is not very self-documenting... "True" vs "False" doesn't mean much in this context. Things like setEnabled(true) / setEnabled(false) are where this works much better.

I'd suggest one of three options:

1) Leave original code as-is. Duplicated code (or nearly so) is nice to avoid, but if it's small and self-evident, it's ok. It's quite likely that if someone added a TabMumble listener in the future, they'd also find it obvious enough to remove it.

2) Add explicit addListeners() / removeListeners() functions (obviously self-documenting), which in turn call a nearby someMethod(true/false) function. If someone goes to look at how addListeners() is implemented, they'll quickly find it's a 1-liner that just bounces the work over to a function a handful of lines nearby.

3) Add an array of relevant events -- ["TabOpen", "TabClose", "TabSelect"], and addListeners/removeListeners that iterate over it to do work.

Personally I'd just go with #1 to keep things simple. #3 would be a runner up, but is generally more useful when there's a bunch of shared entries in the array (or likely to be so in the future).

Dumb-but-simple often wins out over cleverness when it comes to code maintenance. :)

Comment 31

[Josh Matthews [:jdm]]

Sam, can you confirm that you're still working on this?

Comment 32

[Sam Garrett]

(In reply to Josh Matthews [:jdm] from comment #31)
> Sam, can you confirm that you're still working on this?

I have not had the time to look at this. Feel free to reassign.

Comment 33

[Josh Matthews [:jdm]]

Furthermore, could a new mentor who is still involved with this project take over Paul's role?

Comment 34

[sinduja ramaraj]

Hi

I would like to work on this. Can you assign it to me?

Thanks
Sinduja

Comment 35

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.