New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 864836 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 873855



Sign in to add a comment

Compute refresh tab paths without intersections

Project Member Reported by pkasting@chromium.org, Jul 18

Issue description

To save CPU and avoid issues like  bug 864280 , we should try to compute Refresh tab paths without intersections.

There are two cases to consider:
(1) No insets -- this is pretty trivial because nothing really intersects anyway.  Basically just glue the two half-paths we're doing together, without the closing segments in between.
(2) Insets -- this is used when clipping the favicon, but since the favicon is contained entirely in the part of the tab that has vertical sides, we could replace the "with insets" version of this with a different function like "get contents clip path", which in refresh would be a really simple rectangle, and pre-refresh could use the existing insets codepath there.
 
Labels: Group-Tabstrip
Owner: tbergquist@chromium.org
Status: Assigned (was: Available)
Yes!  Great change.  1000x definitely going to do this.
Labels: M-70 Target-70
Owner: dfried@chromium.org
Is this a dupe of  bug 873855 ?
No, I don't think so, though the two probably overlap some.
Cc: thomasanderson@chromium.org pkasting@chromium.org
 Issue 873855  has been merged into this issue.
Note: I was planning on fixing  Issue 873855  in the process of fixing this one.
Status: Started (was: Assigned)
Labels: -M-70 -Target-70 M-71 Target-71
Blockedon: 873855
Labels: -Proj-MdRefresh Proj-DesktopUI
Labels: Hotlist-DesktopUITriaged
Patches that we expect to deliver:
 - Move tab shape calculation to its own class and simplify to a single code path
 - Further simplify clip region for child controls (can be a rectangle)
 - Move all tab rendering parameters into the new class
 - Experiment with using single stroke for the tab border instead of a region
   difference
 - Move all tab painting into the new class

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9

commit e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9
Author: Dana Fried <dfried@chromium.org>
Date: Tue Oct 09 01:21:01 2018

Consolidate tab path computation into a single place.

First of several CLs to move tab rendering out of tab.cc, allowing us to more
easily abstract tab shape.

Eliminates overly-complex interior path calculation.
Unifies border and interior path calculation.

NOTE: Currently there is a #define flag in tab.cc - USE_TAB_STYLE - which
can be used to switch back and forth between the old and new code to verify
that it does not cause regression, as a courtesy to reviewers. The flag
(and all of the old code) will be removed in a follow-up CL that is already
approved for submission.

Followup CLs:
 - Further simplify clip region for child controls (can be a rectangle)
 - Move all tab rendering parameters into tab_style.[h|cc]
 - Experiment with using single stroke for the tab border instead of a region
   difference
 - Move all tab painting into tab_style.[h|cc]

Bug:  864836 
Change-Id: Id22a6ad546c6b53a6164edb6a4a679b51299470c
Reviewed-on: https://chromium-review.googlesource.com/c/1260500
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597765}
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab_strip.h
[add] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab_style.cc
[add] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab_style.h
[modify] https://crrev.com/e12e7bfe42ab9cad7ac9bd558ee472a279e61fb9/chrome/browser/ui/views/tabs/tab_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4dddd717fc2a066345c0a15ed8a64ec1821027bb

commit 4dddd717fc2a066345c0a15ed8a64ec1821027bb
Author: Dana Fried <dfried@chromium.org>
Date: Tue Oct 09 06:22:24 2018

Turns the border stroke for tabs (when present) into a single line.

Previously it was the diff of two complex strokes. This should simplify
rendering. There should be no visual difference when strokes are present at any
DPI scale.

Bug:  864836 
Change-Id: Ie3f72b833103426ef2dec6b654aeaee8077a9f59
Reviewed-on: https://chromium-review.googlesource.com/c/1262400
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597816}
[modify] https://crrev.com/4dddd717fc2a066345c0a15ed8a64ec1821027bb/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/4dddd717fc2a066345c0a15ed8a64ec1821027bb/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/4dddd717fc2a066345c0a15ed8a64ec1821027bb/chrome/browser/ui/views/tabs/tab_style.cc
[modify] https://crrev.com/4dddd717fc2a066345c0a15ed8a64ec1821027bb/chrome/browser/ui/views/tabs/tab_style.h

Project Member

Comment 17 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96482736ce482f06634f93870cc5aeb0dba25880

commit 96482736ce482f06634f93870cc5aeb0dba25880
Author: Dana Fried <dfried@chromium.org>
Date: Tue Oct 09 07:14:05 2018

Move and consolidate tab style logic from tab.cc to tab_style.cc

We also want to (eventually) move rendering logic over, but that's for
another CL.

Bug:  864836 
Change-Id: Id2da530c669358c789ecff4933082615279040a8
Reviewed-on: https://chromium-review.googlesource.com/c/1266726
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597828}
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/ash/tab_scrubber.cc
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_controller.h
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_strip.h
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_style.cc
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_style.h
[modify] https://crrev.com/96482736ce482f06634f93870cc5aeb0dba25880/chrome/browser/ui/views/tabs/tab_unittest.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c910c76084006e6d85502be0f41615ce772cc9f8

commit c910c76084006e6d85502be0f41615ce772cc9f8
Author: Dana Fried <dfried@chromium.org>
Date: Tue Oct 09 09:18:56 2018

Tab interior clip region is now a rectangle, for simplicity.

Bug:  864836 
Change-Id: I41c5f77e78ab2958cb0ad3da1839045716203bd8
Reviewed-on: https://chromium-review.googlesource.com/c/1262422
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597853}
[modify] https://crrev.com/c910c76084006e6d85502be0f41615ce772cc9f8/chrome/browser/ui/views/tabs/tab_style.cc

Status: Fixed (was: Started)
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55f9e8ca5566a029751b7b00341ebb1e0babcd15

commit 55f9e8ca5566a029751b7b00341ebb1e0babcd15
Author: Dana Fried <dfried@chromium.org>
Date: Wed Oct 10 00:36:58 2018

Cleanup of tab_style.cc

As requested by pkasting@

Bug:  864836 
Change-Id: I4b6248e18c82895a858717dea26f8cafc7f34be6
Reviewed-on: https://chromium-review.googlesource.com/c/1272077
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598149}
[modify] https://crrev.com/55f9e8ca5566a029751b7b00341ebb1e0babcd15/chrome/browser/ui/views/tabs/tab_style.cc

Sign in to add a comment