Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967943002:
Temporary fix for "white patch" fullscreen regression on OSX (Closed)
DescriptionTemporary fix for "white patch" fullscreen regression on OSX
This is a partial revert of https://codereview.chromium.org/1790663003
which introduced the bug. While it doesn't fix the regression in popups,
it will at least fix it for a normal browser window which is exposed to
more users.
BUG=604288
Committed: https://crrev.com/07323681ff50e188b664ad404eaf3b04f9add5bc
Cr-Commit-Position: refs/heads/master@{#393027}
Patch Set 1 #Patch Set 2 : #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Temporary fix for a flash fullscreen regression on OSX BUG=604288 ========== to ========== Temporary fix for "white patch" fullscreen regression on OSX BUG=604288 ==========
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
PTAL
rsesek@chromium.org changed reviewers: + avi@chromium.org
This doesn't quite seem like the right fix. What about popups is the problem? Also, adding avi@ as a reviewer since he's explicitly listed in the OWNERS file for these directories.
On 2016/05/11 15:39:41, Robert Sesek wrote: > This doesn't quite seem like the right fix. What about popups is the problem? > > Also, adding avi@ as a reviewer since he's explicitly listed in the OWNERS file > for these directories. Sorry, this is a bit confusing. This is a partial revert of https://chromium.googlesource.com/chromium/src/+/7063f0ac01f527e9bba1fb247d79... which introduced the regression. The reason why this only affect popups is because unlike normal browser windows, the view hierarchy in a popup does not have the content view replaced with a FullSizeContentView (which blocks unwanted immediate frames). This is unfortunately very hacky and should be a short term solution. I've been working on a long term solution, but we're getting close to stable date. If you're not comfortable with this, I can try to see if I can push the Release blocker back (IMO the regression isn't really serious considering what the CL has fixed)
On 2016/05/11 17:27:41, spqchan wrote: > Sorry, this is a bit confusing. This is a partial revert of > https://chromium.googlesource.com/chromium/src/+/7063f0ac01f527e9bba1fb247d79... > which introduced the regression. Can you put this revert info into the description?
Can you just use blockFullscreenResize for this?
Description was changed from ========== Temporary fix for "white patch" fullscreen regression on OSX BUG=604288 ========== to ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of R1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ==========
On 2016/05/11 17:45:01, Robert Sesek wrote: > Can you just use blockFullscreenResize for this? [delegate_ shouldResizeContentView] checks the blockFullscreenResize flag
On 2016/05/11 17:50:24, spqchan wrote: > On 2016/05/11 17:45:01, Robert Sesek wrote: > > Can you just use blockFullscreenResize for this? > > [delegate_ shouldResizeContentView] checks the blockFullscreenResize flag Also, description is now updated
LGTM while you figure this out. BTW, that R syntax in the description is weird. I've seen "r381053" which is the commit position, "commit ce3c4cbf6353f9953cc71edfc9f5e49ea4ecd248". If you want to use the review URL, you might want to put the whole URL in.
Description was changed from ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of R1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ========== to ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ==========
Description was changed from ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ========== to ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ==========
On 2016/05/11 17:50:24, spqchan wrote: > On 2016/05/11 17:45:01, Robert Sesek wrote: > > Can you just use blockFullscreenResize for this? > > [delegate_ shouldResizeContentView] checks the blockFullscreenResize flag Right. I was suggesting setting blockFullscreenResize to YES for popups.
On 2016/05/11 18:07:21, Robert Sesek wrote: > On 2016/05/11 17:50:24, spqchan wrote: > > On 2016/05/11 17:45:01, Robert Sesek wrote: > > > Can you just use blockFullscreenResize for this? > > > > [delegate_ shouldResizeContentView] checks the blockFullscreenResize flag > > Right. I was suggesting setting blockFullscreenResize to YES for popups. Ah gotcha. Unfortunately no, since blockFullscreenResize is used in two places: - L246 which explicitly resizes the fullscreen widget and has to be blocked for both normal windows and popups - L128 which only applies to popups However, I can change replace "isPopup" with a flag that is around the lines of "blockFullscreenResizeForPopup"
lgtm if avi is happy
The CQ bit was checked by spqchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967943002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967943002/20001
Message was sent while issue was closed.
Description was changed from ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ========== to ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 ========== to ========== Temporary fix for "white patch" fullscreen regression on OSX This is a partial revert of https://codereview.chromium.org/1790663003 which introduced the bug. While it doesn't fix the regression in popups, it will at least fix it for a normal browser window which is exposed to more users. BUG=604288 Committed: https://crrev.com/07323681ff50e188b664ad404eaf3b04f9add5bc Cr-Commit-Position: refs/heads/master@{#393027} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/07323681ff50e188b664ad404eaf3b04f9add5bc Cr-Commit-Position: refs/heads/master@{#393027} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
