Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519313002:
MacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server. (Closed)
|
Created:
4 years, 1 month ago by tapted Modified:
4 years, 1 month ago Reviewers:
msw CC:
chromium-reviews, tfarina, groby+bubble_chromium.org, rouslan+bubble_chromium.org, msw+watch_chromium.org, hcarmona+bubble_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server.
BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's
fine for everything except BubbleBorder::NO_ASSETS which should never
draw borders or shadows. On Mac, it allows the WindowServer to provide
the shadow instead.
Add BubbleBorder::PaintNoAssets() which just paints transparent pixels
around the edge of the bubble. This ensures that subviews painting over
the bubble corners (such as the signin promo on the bookmark bubble) get
rounded (again).
BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf
tooltips on ChromeOS. This approach keeps those bubbles to spec with
--secondary-ui-md (e.g. per http://crbug.com/595011 ).
BUG=667623, 660018
Committed: https://crrev.com/bf9bf4f69ef97afe4554e489704940db643dac35
Cr-Commit-Position: refs/heads/master@{#434069}
Patch Set 1 #Patch Set 2 : fix insets too #
Total comments: 4
Patch Set 3 : respond to comments #
Messages
Total messages: 24 (18 generated)
Description was changed from ========== MacViews: For MD bubbles, just use the border drawn by the window server. Fix shadow border BUG= ========== to ========== MacViews: For MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BUG=667623 ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews: For MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BUG=667623 ========== to ========== MacViews: For MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews: For MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623 ========== to ========== MacViews: For MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 ==========
Description was changed from ========== MacViews: For MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 ========== to ========== MacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Mike, please take a look
lgtm with nits https://codereview.chromium.org/2519313002/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/2519313002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:512: // Clip out a round rect so the fill and shadow don't draw over the contents nit: leave this comment in PaintMd (and PaintNoAssets), or incorporate it into the function declaration comment. https://codereview.chromium.org/2519313002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:539: SkRRect clip_r_rect = GetClientRect(view); optional nit: rename |r_rect| and nix the copy below.
Thanks Mike! https://codereview.chromium.org/2519313002/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/2519313002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:512: // Clip out a round rect so the fill and shadow don't draw over the contents On 2016/11/22 18:27:22, msw wrote: > nit: leave this comment in PaintMd (and PaintNoAssets), or incorporate it into > the function declaration comment. Done. Updated the header comment for GetClientRect: // Returns the region within |view| representing the client area. This can be // set as a canvas clip to ensure any fill or shadow from the border does not // draw over the contents of the bubble. https://codereview.chromium.org/2519313002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:539: SkRRect clip_r_rect = GetClientRect(view); On 2016/11/22 18:27:22, msw wrote: > optional nit: rename |r_rect| and nix the copy below. Done.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2519313002/#ps40001 (title: "respond to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479857899044680,
"parent_rev": "cb64faa00c59e43d861e019be65af15bee032104", "commit_rev":
"d323020e61afe5adb5245cba41a566e935e80a86"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 ========== to ========== MacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 ========== to ========== MacViews: For "NO_ASSET" MD bubbles, just use the border drawn by the window server. BubbleBorder::PaintMd() currently ignores BubbleBorder::shadow_. That's fine for everything except BubbleBorder::NO_ASSETS which should never draw borders or shadows. On Mac, it allows the WindowServer to provide the shadow instead. Add BubbleBorder::PaintNoAssets() which just paints transparent pixels around the edge of the bubble. This ensures that subviews painting over the bubble corners (such as the signin promo on the bookmark bubble) get rounded (again). BubbleBorder::NO_ASSETS is also used by the fullscreen bubble and shelf tooltips on ChromeOS. This approach keeps those bubbles to spec with --secondary-ui-md (e.g. per http://crbug.com/595011 ). BUG=667623, 660018 Committed: https://crrev.com/bf9bf4f69ef97afe4554e489704940db643dac35 Cr-Commit-Position: refs/heads/master@{#434069} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bf9bf4f69ef97afe4554e489704940db643dac35 Cr-Commit-Position: refs/heads/master@{#434069} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
