Closed Bug 840722 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23
Tracking Status
firefox22 --- wontfix
firefox23 --- fixed

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

Attachments

(1 file, 6 obsolete files)

I think that we'll able to refactor the code if we add a new object which represents the viewport metadata in browser.js.
OS: Mac OS X → Android
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Attachment #713115 - Flags: review?(bugmail.mozilla)
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.
Attachment #713115 - Flags: review?(bugmail.mozilla) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #713115 - Attachment is obsolete: true
Attachment #713784 - Flags: review?(bugmail.mozilla)
Attachment #713784 - Flags: review?(bugmail.mozilla) → review+
Kartikaya, Thank you.
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
Keywords: checkin-needed
> Created attachment 713784 [details] [diff] [review]
> patch v2

This patch conflict bug 830760.
I'll rebase this.
Keywords: checkin-needed
Attached patch patch v3 (obsolete) — Splinter Review
Rebased on bug 830760.
Attachment #713784 - Attachment is obsolete: true
Attachment #713925 - Flags: review?(bugmail.mozilla)
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.
Attachment #713925 - Flags: review?(bugmail.mozilla) → review+
Attached patch patch v4 (obsolete) — Splinter Review
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?
Attachment #714301 - Flags: review?(bugmail.mozilla)
Comment on attachment 714301 [details] [diff] [review]
patch v4

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

I actually prefer the previous version.
Attachment #714301 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 714301 [details] [diff] [review]
patch v4

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #714301 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b3bdec87d963
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Back out of b3bdec87d963 (obsolete) — Splinter Review
[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.
Attachment #715670 - Flags: approval-mozilla-aurora?
Depends on: 842714
Target Milestone: Firefox 21 → ---
Comment on attachment 715670 [details] [diff] [review]
Back out of b3bdec87d963

Approving the backout from Aurora 21.
Attachment #715670 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 843418
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.
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.
(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}]
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?
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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}
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,
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attached patch patch v4 (obsolete) — Splinter Review
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?
Attachment #713925 - Attachment is obsolete: true
Attachment #715670 - Attachment is obsolete: true
Attachment #727804 - Flags: review?(bugmail.mozilla)
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.
Attachment #727804 - Flags: review?(bugmail.mozilla) → review-
c(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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 on attachment 727804 [details] [diff] [review]
patch v4

So I don't forget.
Attachment #727804 - Flags: review- → review?(bugmail.mozilla)
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.
Attachment #727804 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Yeah changing it just being a "struct" might work better.
Attached patch patch v5Splinter Review
Made the patch to introduce the object as "struct".
Attachment #727804 - Attachment is obsolete: true
Attachment #733817 - Flags: review?(bugmail.mozilla)
Comment on attachment 733817 [details] [diff] [review]
patch v5

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

r=me assuming this passes on try. thanks!
Attachment #733817 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36) 
> r=me assuming this passes on try. thanks!

Thank you! I confirmed this patch has passed on try.
https://hg.mozilla.org/mozilla-central/rev/f9b061217ead
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
The old aurora approval in this bug is confusing my bug queries. Just setting flags to shut it up.
You need to log in before you can comment on or make changes to this bug.