Details
- Reviewers
snorp droeh kats - Commits
- Restricted Diffusion Commit
rMOZILLACENTRAL7f4d0af1b7b4: Bug 1506747 - Add GeckoView API for drawing transparent border around the… - Bugzilla Bug ID
- 1506747
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
r+ with comments addressed
| gfx/layers/opengl/CompositorOGL.cpp | ||
|---|---|---|
| 853 | Do we not need to clear the area excluding the border with mClearColor? Or is that handled elsewhere? | |
| gfx/thebes/gfxPrefs.h | ||
| 457 | Use MOZ_WIDGET_ANDROID here for consistency. Or change the Compositor code to #ifdef ANDROID as well. Also add a comment above the first of these new prefs to the effect of "This adds a border around the composited content, which is needed for ..." | |
| gfx/layers/opengl/CompositorOGL.cpp | ||
|---|---|---|
| 580 | This indicates that the values are not really a "border" but more like "compositor origin". Does that sound better to you? Otherwise I would expect the width/height to be reduced by the border values. | |
| mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java | ||
| 142 | It's not about "reporting" the right bit depth is it? More like requesting the context with the highest depth? | |
| 253 | Describe what the border is. | |
| 450 | Need to also set mCompositorBorderX and mCompositorBorderY | |
| 862 | missing mCompositorBorderX and mCompositorBorderY here too. | |
| 885 | Same deal with mCompositorBorderX and mCompositorBorderY. | |
| gfx/layers/opengl/CompositorOGL.cpp | ||
|---|---|---|
| 580 | If the boarder decreased the width/height then the viewport would need to be changed as well. Yes it changes the origin. I don't care what we call it, but the intent was for adding a border to to the content window. | |
| 853 | I'll double check, but I don't think so any more. When APZ first landed, the background color of the root page was not getting drawn. We were unable to figure out why so I added setting the clear color to the background color (it also helps with white flashing in private browsing). But at some point the bug was fixed and the back ground color of the window in now drawn on android like on other platforms. | |
| gfx/thebes/gfxPrefs.h | ||
| 457 | Okay, it is inconsistent because I usually try to match the #define used in the file. I'm not sure why we have both ANDROID and MOZ_WIDGET_ANDROID, maybe an FxOS anachronism? I'll make it consistent across the patch. | |
| mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java | ||
| 142 | It is, we were reporting 24 regardless of what the hardware supported. This meant the context was always created RGB. If the hardware supports 32 bits and we report that, then context is created as RGBA. But I didn't want to hardcode a return of 32 bits in case the hardware didn't actually support it for some reason. | |
| gfx/layers/opengl/CompositorOGL.cpp | ||
|---|---|---|
| 580 | Seconding snorp's comment here -- but from documentation (eg setCompositorBorder) it certainly seems like it's supposed to be a proper border around content and not a compositor origin. | |
Randall and I talked about this some more and I would really like this to instead be additional x and y arguments to GeckoDisplay.surfaceChanged. Randall is going to look into that approach.
| gfx/layers/ipc/CompositorBridgeParent.cpp | ||
|---|---|---|
| 825 | I'd prefer to drop this ifdef and instead do if (mCompositor->AsCompositorOGL()). There's no harm in allowing this on other platforms too, and it seems odd to have the x and y arguments on this function if they're just dropped on non-android platforms. | |
| gfx/layers/opengl/CompositorOGL.cpp | ||
| 579 | and this one | |
| 851 | Keep this ifdef, since this part is fairly specific to this android feature | |
| 1261 | Drop this ifdef too | |
| gfx/layers/opengl/CompositorOGL.h | ||
| 278 | Ditto, drop this ifdef | |
| 314 | And this one | |
| gfx/thebes/gfxPrefs.h | ||
| 459 | typo: border | |
| gfx/thebes/gfxPrefs.h | ||
|---|---|---|
| 460 | I think we should make this part of GeckoDisplay too somehow, but I don't feel strongly enough about it for this patch. | |
Some minor fixes needed, otherwise looks good!
| mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java | ||
|---|---|---|
| 58 | I think you want to check for negative x/y values here and add that requirement to documentation. | |
| 97 | Check for negative x/y values here as well; also I think we should keep parameter naming consistent between screenOriginChanged and onSurfaceChanged. | |
| 109 | Nit: unnecessary asterisk here. | |
| gfx/thebes/gfxPrefs.h | ||
|---|---|---|
| 459 | I don't know why my brain insists on inserting an 'a' in border. | |
| 460 | Yeah, I can add it as a follow up. | |
| mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java | ||
| 58 | I added checks for left and top but will add checks for width and height in follow up: | |
| 97 | Since this is preexisting code, I would rather add the check in a follow up in case some is already passing in a negative value, I don't want to get this patch backed out for it. | |