Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536223003:
Refactor ContentViewClient (3/6) (Closed)
|
Created:
4 years ago by Jinsuk Kim Modified:
4 years ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, mdjones+watch_chromium.org, donnd+watch_chromium.org, jam, nona+watch_chromium.org, twellington+watch_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, James Su, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ContentViewClient (3/6)
Replaced a Chrome-specific interface getDesired{Width,Height}MeasureSpec
with explicit calls onContentView to set the size.
BUG=620172
Committed: https://crrev.com/0c5dcfb3579be17d056ef8738b0bb9b0c3e20e09
Cr-Commit-Position: refs/heads/master@{#436522}
Patch Set 1 : fix tests #
Total comments: 16
Patch Set 2 : removed getDesiredMeasureSpec #
Total comments: 4
Patch Set 3 : getDesired*MeasureSpec only #
Total comments: 6
Patch Set 4 : addressed comments #
Messages
Total messages: 34 (19 generated)
The CQ bit was checked by jinsukkim@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by jinsukkim@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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactor ContentViewClient (3/6) Migrate view-related interface methods to ViewAndroid/Delegate. BuG=620172 BUG= ========== to ========== Refactor ContentViewClient (3/6) Migrate view-related interface methods to ViewAndroid/Delegate. BUG=620172 ==========
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
only looked at content and ui overall, ViewAndroidDelegate should only be used from ViewAndroid, not from content (if possible) https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (left): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:159: public int getDesiredWidthMeasureSpec() { hmm, probably worthwhile to dig up why this was added as a callback from content, this just seems like a super dumb way to set a view's size these should just be explicit calls on ContentView to set its size instead https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:705: public ViewAndroidDelegate getViewDelegate() { should not need this, this is supplied by the embedder, it should never have to ask for it https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:917: return mViewportHeightPix + mViewAndroidDelegate.getSystemWindowInsetBottom(); these moved calls should be RWHVA->ViewAndroid->ViewAndroidDelegate, it should not involve CVC at all https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1982: mViewAndroidDelegate.onTopControlsChanged(topBarTranslate, topBarShownPix); how hard is it to go through ViewAndroid for these two calls, rather than CVC? I think eventually, logic in this method should move to RWHVA and/or ViewAndroid, perhaps should do that now? https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android... ui/android/view_android.h:15: typedef unsigned int SkColor; include instead
I haven't completed updating the patch yet. Will soon get back to you with it. > only looked at content and ui chrome/ is still messy. Please do take a glimpse yet :/ > overall, ViewAndroidDelegate should only be used from ViewAndroid, not from content (if possible) I'm aware of this. I'll try to avoid it unless it looks impossible. Your feedback is all valid but I'd like to make all those changes you suggested in multiple steps rather than doing it in this CL, to make the scope of work manageable. One change leads to another and it easily makes the size bigger. Though what's done may look half-done or left in intermediate phase but will keep working on to finish migration. https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (left): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:159: public int getDesiredWidthMeasureSpec() { On 2016/11/30 23:22:35, boliu wrote: > hmm, probably worthwhile to dig up why this was added as a callback from > content, this just seems like a super dumb way to set a view's size > > these should just be explicit calls on ContentView to set its size instead It was introduced to customize the view/physical backing size of contextual search panel (of equal or smaller width/height), and now used for overlay panel in general. This is for Chrome only, so doesn't have to be in content layer. This is used in ContentView.onMeasure (for view size), CompositorViewHolder.adjustPhysicalBackingSize. I'm not sure how to make a explicit calls for these... what I can think of alternatively is define OverlayContentView inheriting from ContentView and override its onMeasure to define its size. For physical backing size, define a Chrome-only helper that contains the size info and inject it into CompositorViewHolder. Let me know if it sound plausible, or please elaborate your idea to help me get it better. https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:705: public ViewAndroidDelegate getViewDelegate() { On 2016/11/30 23:22:35, boliu wrote: > should not need this, this is supplied by the embedder, it should never have to > ask for it It was added mostly for getDesiredMeasureSpec*() so can be reverted I think. I'm also aware of your point (see the TODO) And one test Ichrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java) needs to use it by way of viewCoreRef.get().getViewDelegate().getSystemWindowInsetBottom() will it be okay to keep and mark it VisibleForTesting if this is necessary? https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:917: return mViewportHeightPix + mViewAndroidDelegate.getSystemWindowInsetBottom(); On 2016/11/30 23:22:35, boliu wrote: > these moved calls should be RWHVA->ViewAndroid->ViewAndroidDelegate, it should > not involve CVC at all I also had tried so that VAD will handle |getViewportHeightWidhtOSKHiddenPix|. It means mViewportHeightPix needs to move VAD too. So does |getViewportHeightPix()|. Then 4 other classes should be updated and access VAD to get the value... That made me decide to make this intermediate change and take care of the rest better in follow up CLs. https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1982: mViewAndroidDelegate.onTopControlsChanged(topBarTranslate, topBarShownPix); On 2016/11/30 23:22:35, boliu wrote: > how hard is it to go through ViewAndroid for these two calls, rather than CVC? > > I think eventually, logic in this method should move to RWHVA and/or > ViewAndroid, perhaps should do that now? Not impossible but hard to do in one step. I'm afraid it will make this CL pretty big and somewhat beyond the scope I had in mind for this CL. |updateFrameInfo| does many things here - view/scroll update - ImeAdpater - RenderCoordinate - Popup zoomer to name a few. Eventually most of these will move to ViewAndroid like you said but I'd like to do it in several steps. If you really want to see the migration begin, what I can do is to duplicate the call |content_view_core_->UpdateFrameInfo| in RWHVA::OnFrameMetadataUpdated() and do |view_android.UpdateFrameInfo| too, and call up VAD only when top/bottom bar has changed. https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android... ui/android/view_android.h:15: typedef unsigned int SkColor; On 2016/11/30 23:22:35, boliu wrote: > include instead I was a bit puzzled because #include "third_party/skia/include/core/SkColor.h" doesn't work - compiler gave an error not finding other skia header. Then I saw there are many other files do typedef like this for SkColor and just followed them. Will take another look.
boliu@chromium.org changed reviewers: + mdjones@chromium.org
On 2016/12/01 07:59:38, Jinsuk Kim wrote: > I haven't completed updating the patch yet. Will soon get back to you with it. > > > > only looked at content and ui > > chrome/ is still messy. Please do take a glimpse yet :/ I can't tell whether you want me to look at chrome or not. Still haven't looked yet.. > > > overall, ViewAndroidDelegate should only be used from ViewAndroid, not from > content (if possible) > > I'm aware of this. I'll try to avoid it unless it looks impossible. > > Your feedback is all valid but I'd like to make all those changes you suggested > in multiple steps rather than doing it in this CL, to make the scope of work > manageable. One change leads to another and it easily makes the size bigger. > Though what's done may look half-done or left in intermediate phase but will > keep working on to finish migration. Another way to split this is by methods. Eg I think the background color part in this CL is pretty much good to go. Can some of the other method(s) be split off, and moved to RWHVA individually? https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (left): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:159: public int getDesiredWidthMeasureSpec() { On 2016/12/01 07:59:38, Jinsuk Kim wrote: > It was introduced to customize the view/physical backing size of contextual > search panel (of equal or smaller width/height), and now used for overlay panel > in general. This is for Chrome only, so doesn't have to be in content layer. > > This is used in ContentView.onMeasure (for view size), > CompositorViewHolder.adjustPhysicalBackingSize. Looking at adjustPhysicalBackingSize, that's a classic example of why this API is odd. Why is there a setter for PhysicalBackingSize that sometimes just ignores the value that's passed in. That's just totally surprising behavior for anyone who don't read the implementation. > I'm not sure how to make a > explicit calls for these... what I can think of alternatively is define > OverlayContentView inheriting from ContentView and override its onMeasure to > define its size. > > For physical backing size, define a Chrome-only helper that contains the size > info and inject it into CompositorViewHolder. Let me know if it sound plausible, > or please elaborate your idea to help me get it better. +mdjones https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:705: public ViewAndroidDelegate getViewDelegate() { On 2016/12/01 07:59:38, Jinsuk Kim wrote: > On 2016/11/30 23:22:35, boliu wrote: > > should not need this, this is supplied by the embedder, it should never have > to > > ask for it > > It was added mostly for getDesiredMeasureSpec*() so can be reverted I think. I'm > also aware of your point (see the TODO) > > And one test > Ichrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java) > needs to use it by way of > viewCoreRef.get().getViewDelegate().getSystemWindowInsetBottom() will it be okay > to keep and mark it VisibleForTesting if this is necessary? For the test, you can just have Tab keep a ref, and return it? https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android... ui/android/view_android.h:15: typedef unsigned int SkColor; On 2016/12/01 07:59:38, Jinsuk Kim wrote: > On 2016/11/30 23:22:35, boliu wrote: > > include instead > > I was a bit puzzled because #include "third_party/skia/include/core/SkColor.h" > doesn't work - compiler gave an error not finding other skia header. Then I saw > there are many other files do typedef like this for SkColor and just followed > them. Will take another look. Yeah that's really odd. should figure out why include doesn't work
Patchset #2 (id:40001) has been deleted
> Another way to split this is by methods. Eg I think the background color part in > this CL is pretty much good to go. Can some of the other method(s) be split off, > and moved to RWHVA individually? Yes - handled background color in this CL because it was in ContentViewClient and also because it is somewhat isolated from the rest of the data. Other view-related data are intertwined and I believe the work of migrating them is worth another CL. The other two you mentioned leads to many other changes like: getSystemWindowInsetBottom: needs mViewportHeightPix to be moved, and it involves updating other 4 classes, CVC.onSizeChanged too. For this, event handler (java -> native, WIP) needs to be resolved first. onTop/BottomControlsChanged: needs updateFrameInfo duplicated both in CVC/VA in transition period. https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (left): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:159: public int getDesiredWidthMeasureSpec() { On 2016/12/01 23:57:52, boliu wrote: > On 2016/12/01 07:59:38, Jinsuk Kim wrote: > > It was introduced to customize the view/physical backing size of contextual > > search panel (of equal or smaller width/height), and now used for overlay > panel > > in general. This is for Chrome only, so doesn't have to be in content layer. > > > > This is used in ContentView.onMeasure (for view size), > > CompositorViewHolder.adjustPhysicalBackingSize. > > Looking at adjustPhysicalBackingSize, that's a classic example of why this API > is odd. Why is there a setter for PhysicalBackingSize that sometimes just > ignores the value that's passed in. That's just totally surprising behavior for > anyone who don't read the implementation. > > > I'm not sure how to make a > > explicit calls for these... what I can think of alternatively is define > > OverlayContentView inheriting from ContentView and override its onMeasure to > > define its size. > > > > For physical backing size, define a Chrome-only helper that contains the size > > info and inject it into CompositorViewHolder. Let me know if it sound > plausible, > > or please elaborate your idea to help me get it better. > > +mdjones Removed the interfaces getDesired*MeasureSpec() and add an api to ContentView to set the desired size directly. Backing size update can make use of that too. It's much simpler overall. And OverlayPanel doesn't need to provide ContentViewClient any more. https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2536223003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:705: public ViewAndroidDelegate getViewDelegate() { On 2016/12/01 23:57:52, boliu wrote: > On 2016/12/01 07:59:38, Jinsuk Kim wrote: > > On 2016/11/30 23:22:35, boliu wrote: > > > should not need this, this is supplied by the embedder, it should never have > > to > > > ask for it > > > > It was added mostly for getDesiredMeasureSpec*() so can be reverted I think. > I'm > > also aware of your point (see the TODO) > > > > And one test > > > Ichrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java) > > needs to use it by way of > > viewCoreRef.get().getViewDelegate().getSystemWindowInsetBottom() will it be > okay > > to keep and mark it VisibleForTesting if this is necessary? > > For the test, you can just have Tab keep a ref, and return it? Ah right that would do. Done. https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2536223003/diff/20001/ui/android/view_android... ui/android/view_android.h:15: typedef unsigned int SkColor; On 2016/12/01 23:57:52, boliu wrote: > On 2016/12/01 07:59:38, Jinsuk Kim wrote: > > On 2016/11/30 23:22:35, boliu wrote: > > > include instead > > > > I was a bit puzzled because #include "third_party/skia/include/core/SkColor.h" > > doesn't work - compiler gave an error not finding other skia header. Then I > saw > > there are many other files do typedef like this for SkColor and just followed > > them. Will take another look. > > Yeah that's really odd. should figure out why include doesn't work Turns out the error happened because device/power_save_blocker that includes view_android.h has to have //skia, which was absent in its build gn. Fixed.
On 2016/12/02 03:52:07, Jinsuk Kim wrote: > > Another way to split this is by methods. Eg I think the background color part > in > > this CL is pretty much good to go. Can some of the other method(s) be split > off, > > and moved to RWHVA individually? > > Yes - handled background color in this CL because it was in ContentViewClient > and also because it is somewhat isolated from the rest of the data. Other > view-related data are intertwined and I believe the work of migrating them is > worth another CL. > > The other two you mentioned leads to many other changes like: > > getSystemWindowInsetBottom: needs mViewportHeightPix to be moved, and it > involves updating other 4 classes, CVC.onSizeChanged too. For this, event > handler (java -> native, WIP) needs to be resolved first. Ok, I think that implies this stuff is not ready to be moved off of ContentViewClient yet. And that's fine. The point of the refactor is to actually refactor the underlying code to make more sense. imo superficially moving calls to different interfaces is making things more confusing; and it's possible we'll make a mistake in the interface, if we don't have the new refactored implementation to go along with it. Right now, onBackgroundColorChanged looks fine on its own. Create a separate CL. The DesiredMeasureSpec stuff looks fine on its own. Create a separate CL. Everything looks to requires deeper refactors, so do those deeper refactors first. https://codereview.chromium.org/2536223003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/2536223003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:114: if (mDesiredWidth != 0 && mDesiredHeight != 0) { Use MeasureSpec.UNSPECIFIED instead of 0, this also means need to intialize vars to MeasureSpec.UNSPECIFIED also this implies calling setDesiredSize must set *both* width and height, seems like an unnecessary restriction? https://codereview.chromium.org/2536223003/diff/60001/device/power_save_block... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2536223003/diff/60001/device/power_save_block... device/power_save_blocker/BUILD.gn:54: "//skia", err, this makes no sense to me, why does this need to depend on skia?
Description was changed from
==========
Refactor ContentViewClient (3/6)
Migrate view-related interface methods to ViewAndroid/Delegate.
BUG=620172
==========
to
==========
Refactor ContentViewClient (3/6)
Replaced a Chrome-specific interface getDesired{Width,Height}MeasureSpec
with explicit calls onContentView to set the size.
BUG=620172
==========
> Right now, onBackgroundColorChanged looks fine on its own. > Create a separate CL. > The DesiredMeasureSpec stuff looks fine on its own. Create > a separate CL. Left only DesiredMeasureSpec stuff in this CL. https://codereview.chromium.org/2536223003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/2536223003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:114: if (mDesiredWidth != 0 && mDesiredHeight != 0) { On 2016/12/03 00:16:39, boliu wrote: > Use MeasureSpec.UNSPECIFIED instead of 0, this also means need to intialize vars > to MeasureSpec.UNSPECIFIED I guess you meant MeasureSpec(0, MeasureSpec.UNSPECIFIED). Defined a default value indicating the value need not overriding. > > also this implies calling setDesiredSize must set *both* width and height, seems > like an unnecessary restriction? Checked each width and height and override separately (when they're not the default value) https://codereview.chromium.org/2536223003/diff/60001/device/power_save_block... File device/power_save_blocker/BUILD.gn (right): https://codereview.chromium.org/2536223003/diff/60001/device/power_save_block... device/power_save_blocker/BUILD.gn:54: "//skia", On 2016/12/03 00:16:39, boliu wrote: > err, this makes no sense to me, why does this need to depend on skia? Because it has a reference to teh type SkColor. I don't know for sure but that's probably why there are so many one-liner 'typedef unsigned int SkColor' instead of adding dependency on //skia.
OverlayPanel lgtm % nit https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:245: int width = mContentViewWidth == 0 ? ContentView.DEFAULT_MEASURE_SPEC extra space: "int width = mCon..."
https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:920: ContentView cv = (ContentView) contentViewCore.getContainerView(); can OverlayPanelContent also directly set the width height here? then don't need the getters on ContentView https://codereview.chromium.org/2536223003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/2536223003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:47: private int mDesiredWidth = DEFAULT_MEASURE_SPEC; call these mDesiredWidth*MeasureSpec*, too easy to mistake these for actual width/height
https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java (right): https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java:920: ContentView cv = (ContentView) contentViewCore.getContainerView(); On 2016/12/05 23:57:46, boliu wrote: > can OverlayPanelContent also directly set the width height here? then don't need > the getters on ContentView Done. https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2536223003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:245: int width = mContentViewWidth == 0 ? ContentView.DEFAULT_MEASURE_SPEC On 2016/12/05 17:58:50, mdjones wrote: > extra space: "int width = mCon..." Done. https://codereview.chromium.org/2536223003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/2536223003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:47: private int mDesiredWidth = DEFAULT_MEASURE_SPEC; On 2016/12/05 23:57:46, boliu wrote: > call these mDesiredWidth*MeasureSpec*, too easy to mistake these for actual > width/height Done.
lgtm
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org Link to the patchset: https://codereview.chromium.org/2536223003/#ps100001 (title: "addressed 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": 100001, "attempt_start_ts": 1480994092798830,
"parent_rev": "a1a55ce8c2c6fd43850c651e6fd1178c01150294", "commit_rev":
"3b0dea446fc72da5e592dedf11e4061622a8865d"}
Message was sent while issue was closed.
Description was changed from
==========
Refactor ContentViewClient (3/6)
Replaced a Chrome-specific interface getDesired{Width,Height}MeasureSpec
with explicit calls onContentView to set the size.
BUG=620172
==========
to
==========
Refactor ContentViewClient (3/6)
Replaced a Chrome-specific interface getDesired{Width,Height}MeasureSpec
with explicit calls onContentView to set the size.
BUG=620172
==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Refactor ContentViewClient (3/6)
Replaced a Chrome-specific interface getDesired{Width,Height}MeasureSpec
with explicit calls onContentView to set the size.
BUG=620172
==========
to
==========
Refactor ContentViewClient (3/6)
Replaced a Chrome-specific interface getDesired{Width,Height}MeasureSpec
with explicit calls onContentView to set the size.
BUG=620172
Committed: https://crrev.com/0c5dcfb3579be17d056ef8738b0bb9b0c3e20e09
Cr-Commit-Position: refs/heads/master@{#436522}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0c5dcfb3579be17d056ef8738b0bb9b0c3e20e09 Cr-Commit-Position: refs/heads/master@{#436522} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
