840722 - Add an object which represents the viewport metadata in browser.js

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I think that we'll able to refactor the code if we add a new object which represents the viewport metadata in browser.js.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a693cb826abb

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 713115 [details] [diff] [review]
patch

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

Looks nice, thanks! Just a few style nits below.

::: mobile/android/chrome/content/browser.js
@@ +5564,5 @@
> + *   maxZoom (float): The maximum zoom level.
> + *   autoSize (boolean): Resize the CSS viewport when the window resizes.
> + *   allowZoom (boolean): Let the user zoom in or out.
> + *
> + *   scaleRatio (float): The device-pixel-to-CSS-px ratio.

nit: remove the empty line above this

@@ +5566,5 @@
> + *   allowZoom (boolean): Let the user zoom in or out.
> + *
> + *   scaleRatio (float): The device-pixel-to-CSS-px ratio.
> + */
> +function ViewportMetadata (aMetadata = {}) {

nit: remove space before "("

@@ +5578,5 @@
> +
> +  this.scaleRatio = ViewportHandler.getScaleRatio();
> +  Object.seal(this);
> +}
> +ViewportMetadata.prototype = {

nit: remove the empty line above the scaleRatio assignment, and insert an empty line between the ViewportMetadata function and the prototype.

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Kartikaya, Thank you.

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

> Created attachment 713784 [details] [diff] [review]
> patch v2

This patch conflict bug 830760.
I'll rebase this.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v3

Rebased on bug 830760.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 713925 [details] [diff] [review]
patch v3

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

::: mobile/android/chrome/content/browser.js
@@ +5488,5 @@
> +        return new ViewportMetadata({
> +          defaultZoom: 1,
> +          autoSize: true,
> +          allowZoom: true
> +        });

Good catch on these; I should have noticed that on the first review.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v4

I'm sorry I asked many times.

I think this style is more clear than patch v3 for assignment default value.
How about do you think?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 714301 [details] [diff] [review]
patch v4

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

I actually prefer the previous version.

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 714301 [details] [diff] [review]
patch v4

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Comment on attachment 714301 [details] [diff] [review]
> patch v4
> 
> Review of attachment 714301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I actually prefer the previous version.

OK. Thank you.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3bdec87d963

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3bdec87d963

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Backed out for causing bug 842418: https://hg.mozilla.org/integration/mozilla-inbound/rev/7210fcb55aae

Note: the backout is on FF22 but the original landing was on FF21; so the backout will need to be uplifted to FF21 as well.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Back out of b3bdec87d963

[Approval Request Comment]
The patch from this bug introduced some exceptions (see bug 842418) so I backed it out. The backout needs to be uplifted to aurora since the original patch landed pre-merge on FF21 and the backout landed post-merge on FF22.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7210fcb55aae

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 715670 [details] [diff] [review]
Back out of b3bdec87d963

Approving the backout from Aurora 21.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Backed out on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/60d687faf510

Comment 19

[Kevin Brosnan [:kbrosnan]]

The patch also affected web apps. I was able to pinch zoom in the Twitter web app when this was checked in. When verifying please make sure the Twitter web app does not allow zooming.

Comment 20

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I tried to debug why this patch causes the error with adding some log. As the result, it was very mystery...

* Only this patch (https://tbpl.mozilla.org/?tree=Try&rev=1f16e2486b56), or this patch + logging in java code (https://tbpl.mozilla.org/?tree=Try&rev=057a3e709715):
------
E/GeckoApp( 4984): Exception handling message "Tab:ViewportMetadata":
E/GeckoApp( 4984): org.json.JSONException: Value null at minZoom of type org.json.JSONObject$1 cannot be converted to double
E/GeckoApp( 4984): 	at org.json.JSON.typeMismatch(JSON.java:96)
E/GeckoApp( 4984): 	at org.json.JSONObject.getDouble(JSONObject.java:412)
E/GeckoApp( 4984): 	at org.mozilla.gecko.ZoomConstraints.<init>(ZoomConstraints.java:27)
E/GeckoApp( 4984): 	at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:860)
E/GeckoApp( 4984): 	at org.mozilla.gecko.BrowserApp.handleMessage(BrowserApp.java:490)
E/GeckoApp( 4984): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:75)
E/GeckoApp( 4984): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1811)
E/GeckoApp( 4984): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoApp( 4984): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoApp( 4984): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:305)
E/GeckoApp( 4984): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:113)
E/GeckoApp( 4984): Exception handling message "{"minZoom":null,"allowZoom":true,"maxZoom":null,"type":"Tab:ViewportMetadata","defaultZoom":1,"tabID":0}
--------
* And I tried this patch with --disable-ion (https://tbpl.mozilla.org/?tree=Try&rev=6fbeeec93773), this error is reproducible.

* But When is this patch + logging in java + logging in JS (https://tbpl.mozilla.org/?tree=Try&rev=3e52da2d5a06), there is no error.

From the above result, this error might be caused by JS engine's bug.
We need report about this bug to it.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #20)
> * But When is this patch + logging in java + logging in JS
> (https://tbpl.mozilla.org/?tree=Try&rev=3e52da2d5a06), there is no error.
> 


There is an error, but it's a JS error that causes sendViewportMetadata to fail out, and therefore hides the Java error.

02-21 21:35:26.268 E/GeckoConsole( 9923): [JavaScript Error: "NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIConsoleService.logMessage]" {file: "chrome://browser/content/browser.js" line: 3881}]

Comment 22

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I insert some code to dump internal data & test it on try server.
https://tbpl.mozilla.org/?tree=Try&rev=c95d8a05cf1a

From these logs, org.json.JSONObject.getDouble() cannot accept null value converted from NaN.

Kartikaya, what do you think from these log?

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #22)
> I insert some code to dump internal data & test it on try server.
> https://tbpl.mozilla.org/?tree=Try&rev=c95d8a05cf1a
> 
> From these logs, org.json.JSONObject.getDouble() cannot accept null value
> converted from NaN.

Yes, that is correct. The JS code shouldn't be sending NaNs.

> 
> Kartikaya, what do you think from these log?

Which logs do you want me to look at specifically? That try push has a lot of tests and logs.

Comment 24

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Which logs do you want me to look at specifically? That try push has a lot
> of tests and logs.

Oh. Sorry. I forgot to paste the log.
Examples are following (include some dump code which I inserted).


===========
From Android 4.0 Panda try talos remote-trobocheck:
-----------
E/GeckoConsole( 2821): getViewportMetadata: pass-3
E/GeckoConsole( 2821): updateViewportMetadata-2: ({width:NaN, height:NaN, defaultZoom:NaN, minZoom:NaN, maxZoom:NaN, autoSize:false, allowZoom:true, isSpecified:false, scaleRatio:1})
E/GeckoConsole( 2821): updateViewportMetadata-2: ({width:NaN, height:NaN, defaultZoom:NaN, minZoom:NaN, maxZoom:NaN, autoSize:false, allowZoom:true, isSpecified:false, scaleRatio:1})
W/GeckoGlobalHistory( 2821): Rebuilding visited link set...
E/GeckoConsole( 2821): Tab-ViewportMetadata: about:home
E/GeckoConsole( 2821): Tab-ViewportMetadata: ({width:NaN, height:NaN, defaultZoom:NaN, minZoom:NaN, maxZoom:NaN, autoSize:false, allowZoom:true, isSpecified:false, scaleRatio:1})
E/GeckoConsole( 2821): Tab-ViewportMetadata: stacktrace: sendViewportMetadata@chrome://browser/content/browser.js:4038
E/GeckoConsole( 2821): updateViewportMetadata@chrome://browser/content/browser.js:3920
E/GeckoConsole( 2821): updateMetadata@chrome://browser/content/browser.js:5594
E/GeckoConsole( 2821): @chrome://browser/content/browser.js:4092
E/GeckoApp( 2821): Exception handling message "Tab:ViewportMetadata":
E/GeckoApp( 2821): org.json.JSONException: Value null at minZoom of type org.json.JSONObject$1 cannot be converted to double
E/GeckoApp( 2821): 	at org.json.JSON.typeMismatch(JSON.java:100)
E/GeckoApp( 2821): 	at org.json.JSONObject.getDouble(JSONObject.java:412)
E/GeckoApp( 2821): 	at org.mozilla.gecko.ZoomConstraints.<init>(ZoomConstraints.java:27)
E/GeckoApp( 2821): 	at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:789)
E/GeckoApp( 2821): 	at org.mozilla.gecko.BrowserApp.handleMessage(BrowserApp.java:873)
E/GeckoApp( 2821): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:75)
E/GeckoApp( 2821): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1833)
E/GeckoApp( 2821): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoApp( 2821): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoApp( 2821): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:286)
E/GeckoApp( 2821): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:104)
E/GeckoApp( 2821): Exception handling message "{"minZoom":null,"allowZoom":true,"maxZoom":null,"type":"Tab:ViewportMetadata","uri":"about:home","defaultZoom":1,"tabID":0}


===========
Android 4.0 Panda try talos remote-ts
-----------
E/GeckoConsole( 3230): getViewportMetadata: pass-3
E/GeckoConsole( 3230): updateViewportMetadata-2: ({width:NaN, height:NaN, defaultZoom:NaN, minZoom:NaN, maxZoom:NaN, autoSize:false, allowZoom:true, isSpecified:false, scaleRatio:1})
E/GeckoConsole( 3230): updateViewportMetadata-2: ({width:NaN, height:NaN, defaultZoom:NaN, minZoom:NaN, maxZoom:NaN, autoSize:false, allowZoom:true, isSpecified:false, scaleRatio:1})
E/GeckoConsole( 3230): Tab-ViewportMetadata: http://bm-remote.build.mozilla.org/startup_test/startup_test.html?begin=1363587687848
E/GeckoConsole( 3230): Tab-ViewportMetadata: ({width:NaN, height:NaN, defaultZoom:NaN, minZoom:NaN, maxZoom:NaN, autoSize:false, allowZoom:true, isSpecified:false, scaleRatio:1})
E/GeckoConsole( 3230): Tab-ViewportMetadata: stacktrace: sendViewportMetadata@chrome://browser/content/browser.js:4038
E/GeckoConsole( 3230): updateViewportMetadata@chrome://browser/content/browser.js:3920
E/GeckoConsole( 3230): updateMetadata@chrome://browser/content/browser.js:5594
E/GeckoConsole( 3230): @chrome://browser/content/browser.js:4092
E/GeckoApp( 3230): Exception handling message "Tab:ViewportMetadata":
E/GeckoApp( 3230): org.json.JSONException: Value null at minZoom of type org.json.JSONObject$1 cannot be converted to double
E/GeckoApp( 3230): 	at org.json.JSON.typeMismatch(JSON.java:100)
E/GeckoApp( 3230): 	at org.json.JSONObject.getDouble(JSONObject.java:412)
E/GeckoApp( 3230): 	at org.mozilla.gecko.ZoomConstraints.<init>(ZoomConstraints.java:27)
E/GeckoApp( 3230): 	at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:789)
E/GeckoApp( 3230): 	at org.mozilla.gecko.BrowserApp.handleMessage(BrowserApp.java:873)
E/GeckoApp( 3230): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:75)
E/GeckoApp( 3230): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1833)
E/GeckoApp( 3230): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoApp( 3230): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoApp( 3230): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:286)
E/GeckoApp( 3230): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:104)
E/GeckoApp( 3230): Exception handling message "{"minZoom":null,"allowZoom":true,"maxZoom":null,"type":"Tab:ViewportMetadata","uri":"http:\/\/bm-remote.build.mozilla.org\/startup_test\/startup_test.html?begin=1363587687848","defaultZoom":1,"tabID":0}

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Well that seems to make sense. minZoom is calculated to be NaN and it's never corrected. Probably you want to remove this part of the patch:


    1.86 -      minZoom: metadata.minZoom || 0,
    1.87 -      maxZoom: metadata.maxZoom || 0,
    1.88 +      minZoom: metadata.minZoom,
    1.89 +      maxZoom: metadata.maxZoom,

Comment 26

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> Well that seems to make sense. minZoom is calculated to be NaN and it's
> never corrected. Probably you want to remove this part of the patch:
> 
> 
>     1.86 -      minZoom: metadata.minZoom || 0,
>     1.87 -      maxZoom: metadata.maxZoom || 0,
>     1.88 +      minZoom: metadata.minZoom,
>     1.89 +      maxZoom: metadata.maxZoom,

Thank you. I removed its part from the patch, It's don't dump any error!
https://tbpl.mozilla.org/?tree=Try&rev=5413658ac7ee

I'll attach the new patch.

Comment 27

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v4

Rebased on latest mozilla-central!
try-server: https://tbpl.mozilla.org/?tree=Try&rev=3a93fdd693c2

I have one question. If we validate the given value in setter like this patch, should we check whether the value is null or not? At now, I think that we need not do it from the current code in our source tree. How do you think about it?

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 727804 [details] [diff] [review]
patch v4

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

I thought the main advantage of previous versions of your patch was the the ViewportMetadata object was immutable once created. With this new patch it looks like there isn't really much advantage to adding ViewportMetadata at all. Can you explain why you think this is better? Minusing for now to remove it from my queue but I can re-evaluate it once you respond.

Comment 29

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

c(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> I thought the main advantage of previous versions of your patch was the the
> ViewportMetadata object was immutable once created. With this new patch it
> looks like there isn't really much advantage to adding ViewportMetadata at
> all. Can you explain why you think this is better? Minusing for now to
> remove it from my queue but I can re-evaluate it once you respond.

I apologize that my replying is late.
I thought that, If we introduce a new object which represent viewport metadata, we can encapsulate data assignment. This means we can increase the safety. This change can  gathersome magic number which is some place in our codes.
For example, we can simplize & encapsulate like this:
-      minZoom: metadata.minZoom || 0,
-      maxZoom: metadata.maxZoom || 0,
+      minZoom: metadata.minZoom,
+      maxZoom: metadata.maxZoom,

And, the new patch is also immutable. The new patch seals the ViewportMetadata object as no adding any property too. This is same behavior as the old one.

Of cource, the new one has some demerit:
* This is not simple like only struct.
* We always need to call getter/setter for access property. This is trade-off for safety & validation values.

How do you think?

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Oh, sorry, I think I misread the patch when I looked through it last time. I'll review it again, hopefully better this time.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 727804 [details] [diff] [review]
patch v4

So I don't forget.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 727804 [details] [diff] [review]
patch v4

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

r=me with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +5756,5 @@
> +  _allowZoom: null,
> +  _isSpecified: null,
> +  scaleRatio: null,
> +
> +  get width () {

nit: Remove the space before the (). Same goes for all the getters/setters.

@@ +5761,5 @@
> +    return this._width;
> +  },
> +  set width (v) {
> +    let isValid = (v !== undefined) && !isNaN(v);
> +    return this._width = isValid ? v : 0;

For the setters, I feel like if we're going to all this trouble to check for NaN-ness we should also check for number-ness. Right now if you do "metadata.width = true", that will pass the "isValid" check and get assigned to this._width, which seems wrong. I'm not sure if it's worth doing this, but if it's easy then we might as well.

Comment 33

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> For the setters, I feel like if we're going to all this trouble to check for
> NaN-ness we should also check for number-ness. Right now if you do
> "metadata.width = true", that will pass the "isValid" check and get assigned
> to this._width, which seems wrong. I'm not sure if it's worth doing this,
> but if it's easy then we might as well.

Your said is just reasonable. In the patch (attachment 727804 [details] [diff] [review]), I assumed the" isValid" check tightly from our codes. For example, I assume there is no part of that "metadata.width" will be set boolean value from the current our code. Because:

* If it'll be passed any boolean value in the future, it's caused by a new changeset in such case.
* This assuming can decrease the operation of validation in setters. If we check full validation, the operation of it will be "slightly" large.

But this assuming was not "safety" perfectly. At now, I think it may be meaningless. I'm thinking that we introduce only the object as "struct" at this bug.

Kartikaya, how about do you think?

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Yeah changing it just being a "struct" might work better.

Comment 35

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v5

Made the patch to introduce the object as "struct".

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 733817 [details] [diff] [review]
patch v5

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

r=me assuming this passes on try. thanks!

Comment 37

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #36) 
> r=me assuming this passes on try. thanks!

Thank you! I confirmed this patch has passed on try.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b061217ead

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9b061217ead

Comment 40

[Ryan VanderMeulen [:RyanVM]]

The old aurora approval in this bug is confusing my bug queries. Just setting flags to shut it up.