Closed
Bug 840722
Opened 9 years ago
Closed 9 years ago
Add an object which represents the viewport metadata in browser.js
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox22 wontfix, firefox23 fixed)
RESOLVED
FIXED
Firefox 23
People
(Reporter: tetsuharu, Assigned: tetsuharu)
References
Details
Attachments
(1 file, 6 obsolete files)
9.73 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
I think that we'll able to refactor the code if we add a new object which represents the viewport metadata in browser.js.
Assignee | ||
Updated•9 years ago
|
OS: Mac OS X → Android
Hardware: x86 → All
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #713115 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 2•9 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=a693cb826abb
Comment 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #713115 -
Attachment is obsolete: true
Attachment #713784 -
Flags: review?(bugmail.mozilla)
Updated•9 years ago
|
Attachment #713784 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Kartikaya, Thank you.
Assignee | ||
Comment 6•9 years ago
|
||
> Created attachment 713784 [details] [diff] [review] > patch v2 This patch conflict bug 830760. I'll rebase this.
Keywords: checkin-needed
Assignee | ||
Comment 7•9 years ago
|
||
Rebased on bug 830760.
Attachment #713784 -
Attachment is obsolete: true
Attachment #713925 -
Flags: review?(bugmail.mozilla)
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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-
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3bdec87d963
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3bdec87d963
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment 14•9 years ago
|
||
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 → ---
Comment 15•9 years ago
|
||
[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?
Updated•9 years ago
|
Target Milestone: Firefox 21 → ---
Comment 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
Backed out on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/60d687faf510
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
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•9 years ago
|
||
(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}]
Assignee | ||
Comment 22•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 24•9 years ago
|
||
(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}
Comment 25•9 years ago
|
||
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,
Assignee | ||
Comment 26•9 years ago
|
||
(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.
Assignee | ||
Comment 27•9 years ago
|
||
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 28•9 years ago
|
||
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-
Assignee | ||
Comment 29•9 years ago
|
||
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?
Comment 30•9 years ago
|
||
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•9 years ago
|
||
Comment on attachment 727804 [details] [diff] [review] patch v4 So I don't forget.
Attachment #727804 -
Flags: review- → review?(bugmail.mozilla)
Comment 32•9 years ago
|
||
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+
Assignee | ||
Comment 33•9 years ago
|
||
(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?
Comment 34•9 years ago
|
||
Yeah changing it just being a "struct" might work better.
Assignee | ||
Comment 35•9 years ago
|
||
Made the patch to introduce the object as "struct".
Attachment #727804 -
Attachment is obsolete: true
Attachment #733817 -
Flags: review?(bugmail.mozilla)
Comment 36•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 37•9 years ago
|
||
(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.
Comment 38•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b061217ead
Keywords: checkin-needed
Comment 39•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9b061217ead
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Comment 40•9 years ago
|
||
The old aurora approval in this bug is confusing my bug queries. Just setting flags to shut it up.
status-firefox22:
--- → wontfix
status-firefox23:
--- → fixed
Updated•1 year ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•