856008 - Move ContentAreaObserver out of browser.js and clean the code up a bit

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Comment 1

[Jim Mathies [:jimm]]

Attachment: move patch

Comment 2

[Jim Mathies [:jimm]]

Attachment: move cao out of browser.js

Comment 3

[Jim Mathies [:jimm]]

Attachment: cleanup work

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 731138 [details] [diff] [review]
move cao out of browser.js

This is a simple move of code with an added comment header as well in the file. No functional changes.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 731139 [details] [diff] [review]
cleanup work

Some cleanup work and reorg of the code. One minor functional change here, I've remove the viewable-height and width classes from browsers for now. It's causing annoying shifting when the soft keyboard comes up. Form positioning will be address in follow up work.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 731138 [details] [diff] [review]
move cao out of browser.js

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

Looks good.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 731139 [details] [diff] [review]
cleanup work

I can't claim to really understand the implications of removing those width/height rules. Forwarding to Matt.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 731139 [details] [diff] [review]
cleanup work

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

::: browser/metro/base/content/ContentAreaObserver.js
@@ +141,5 @@
> +    this._disatchBrowserEvent("ViewableSizeChanged");
> +  },
> +
> +  onBrowserCreated: function onBrowserCreated(aBrowser) {
> +    aBrowser.setAttribute("class", "content-width content-height");

Let's do this instead, so we can't end up accidentally overwriting classes added elsewhere:

aBrowser.classList.add("content-width");
aBrowser.classList.add("content-height");

@@ +216,5 @@
> +  _updateTabs: function _updateTabs(aWidth, aHeight) {
> +    for (let idx = Browser.tabs.length - 1; idx >=0; idx--) {
> +      let tab = Browser.tabs[idx];
> +      let oldContentWindowWidth = tab.browser.contentWindowWidth;
> +      tab.updateViewportSize(aWidth, aHeight); // contentWindowWidth may change here.

This whole function currently does nothing; I think we should just remove it (including the associated functions in browser.js, though that can be a separate patch).  When we want to re-add support for <meta name=viewport> we should use the platform code instead (bug 716575).

@@ +233,5 @@
> +
> +  _debugDumpDims: function _debugDumpDims() {
> +    let width = parseInt(this.styles["window-width"].width);
> +    let height = parseInt(this.styles["window-height"].height);
> +    Util.dumpLn("window:", width, height);

You may want to remove this debug code before checkin.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85c51fb7a1ec
https://hg.mozilla.org/mozilla-central/rev/8870e8029899