Closed
Bug 719570
Opened 12 years ago
Closed 12 years ago
Need option to set checkboard to be flat background color for Eideticker
Categories
(Firefox for Android Graveyard :: General, defect, P3)
Tracking
(fennec11+)
RESOLVED
FIXED
Firefox 13
Tracking | Status | |
---|---|---|
fennec | 11+ | --- |
People
(Reporter: wlach, Assigned: wlach)
References
Details
Attachments
(1 file, 2 obsolete files)
10.98 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
The way things work now, I need to use a complicated heuristic for actually detecting checkboarding (see: https://github.com/mozilla/eideticker/blob/master/src/videocapture/videocapture/checkerboard.py). Unfortunately, after some testing, I've found this actually doesn't work that well -- it misdetects certain patterns as being checkerboards. I could certainly try to improve the heuristic, but I'm not sure if it would be that easy or worthwhile. I think an easier route would be to modify Native Fennec to be able to use an arbitrary color for the checkerboard, dictated by a pref. Bug 710533 already contains most of the code needed for this. Once it lands, it should be a simple matter to modify the code to just return an image filled with the right arbitrary colour instead of a checkerboard.
Assignee | ||
Comment 1•12 years ago
|
||
Actually it would probably be better to just use the page's background color, as that's what XUL Fennec and the Android default browser (at least in 2.2) use.
Summary: Need option to set checkboard background to an arbitrary color for Eideticker → Need option to set checkboard to be flat background color for Eideticker
Assignee | ||
Comment 2•12 years ago
|
||
Here's a simple patch to add the option (disabled by default) to use a flat pattern instead of a checkerboard. It's a profile option, not an Android option, because it would be desirable to set it programmatically on a blank eideticker profile before beginning a test. Note this patch is based on top of the currently proposed patch for bug 710533 (which adds a tint to the checkerboard)
Assignee: nobody → wlachance
Attachment #590810 -
Flags: review?(pwalton)
Updated•12 years ago
|
tracking-fennec: --- → 11+
Priority: -- → P3
Comment 3•12 years ago
|
||
Comment on attachment 590810 [details] [diff] [review] Add option to set checkboard to be page background color Review of attachment 590810 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/CheckerboardImage.java @@ +88,5 @@ > + Arrays.fill(fillBuffer, color16); > + > + for (int i = 0; i < SIZE; i++) { > + shortBuffer.put(fillBuffer); > + } nit: return; here to avoid indentation ::: mobile/android/chrome/content/browser.js @@ +814,4 @@ > let computedStyle = contentWindow.getComputedStyle(contentDocument.body); > viewport.backgroundColor = computedStyle.backgroundColor; > } > + viewport.showCheckerboardChecks = Services.prefs.getBoolPref("gfx.show_checkerboard_pattern"); getDrawMetadata() is called all the time, so I'm worried about the performance of calling out to prefs here. Especially since users will almost never modify this, but all users will pay a performance regression for the feature. I'd prefer to make this a separate event that calls out to Gecko and sets a flag in the GeckoSoftwareLayerClient.
Attachment #590810 -
Flags: review?(pwalton) → review-
Assignee | ||
Comment 4•12 years ago
|
||
Update to hopefully address issues. As discussed on irc, I made it so that we read the pref once on startup instead of listening to it change, as that avoids some extra complexity/overhead which isn't worth it for a testing-only feature.
Attachment #590810 -
Attachment is obsolete: true
Attachment #591954 -
Flags: review?(pwalton)
Comment 5•12 years ago
|
||
Comment on attachment 591954 [details] [diff] [review] Add option to set checkboard to be page background color (take 2) Review of attachment 591954 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! ::: mobile/android/base/gfx/LayerController.java @@ +90,5 @@ > private LayerClient mLayerClient; /* The layer client. */ > > /* The new color for the checkerboard. */ > private int mCheckerboardColor; > + private boolean mCheckerboardShowChecks; nit: The name mCheckerboardShouldShowChecks seems a little clearer to me. ::: mobile/android/base/gfx/LayerRenderer.java @@ +354,5 @@ > } > > private void updateCheckerboardLayer(GL10 gl, RenderContext renderContext) { > + int checkerboardColor = mView.getController().getCheckerboardColor(); > + boolean showChecks = mView.getController().getCheckerboardShowChecks(); nit: I'd prefer the method name checkerboardShouldShowChecks(), it seems clearer.
Attachment #591954 -
Flags: review?(pwalton) → review+
Comment 6•12 years ago
|
||
Try run for 565c1a2b49fd is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=565c1a2b49fd Results (out of 52 total builds): exception: 2 success: 45 warnings: 4 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-565c1a2b49fd
Assignee | ||
Comment 7•12 years ago
|
||
This addresses the nits in the review and updates for bitrot. Carrying over r+
Attachment #591954 -
Attachment is obsolete: true
Attachment #593236 -
Flags: review+
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Mozilla RelEng Bot from comment #6) > Try run for 565c1a2b49fd is complete. > Detailed breakdown of the results available here: > https://tbpl.mozilla.org/?tree=Try&rev=565c1a2b49fd > Results (out of 52 total builds): > exception: 2 > success: 45 > warnings: 4 > failure: 1 > Builds (or logs if builds failed) available at: > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla. > com-565c1a2b49fd Looks ok except for some timeouts on the crashtest, which seem to be occurring with other builds on try.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6717b2b6a09c
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Updated•3 years 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
•