Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278423002:
Misc. cleanup (Closed)
|
Created:
4 years, 3 months ago by Peter Kasting Modified:
4 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, vabr+watchlistpasswordmanager_chromium.org, tfarina, noyau+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, gcasto+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMisc. cleanup
* Improve comments
* Eliminate -webkit-transition that seems to do nothing
* Group related CSS selectors near each other in preparation for combining/modifying them
* Don't use RTL folder images on Mac (see https://cs.chromium.org/chromium/src/chrome/browser/resource_delegate_mac.mm?rcl=0&l=50 )
* Use "x .y" instead of "x > * > .y" when the two will select the same elements, for brevity
* Remove unnecessary size arg to getFaviconImageSet() (matches default value)
* Don't use the name "scale_factor" for floats relating to scale, as "scale" and "scale factor" are distinct concepts. Use "scale" when that's what is meant.
* Avoid unnecessary using directives.
* Use a helper function to avoid repeating the same multi-part conditional
* NULL -> nullptr
* Improve function and variable names
* Eliminate NOTREACHED() that can, in fact, be reached
BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7f8dd6be8d00e838b371f904ddf2264172fdd826
Cr-Commit-Position: refs/heads/master@{#416425}
Patch Set 1 #Patch Set 2 : Fixes #Patch Set 3 : Forgot a few #
Total comments: 2
Patch Set 4 : Reinstate incorrect hack for now (will remove separately) #Patch Set 5 : Further cleanup in preparation for removing hack in subsequent patch #
Total comments: 15
Patch Set 6 : Review comments #
Total comments: 11
Patch Set 7 : Remove dead rules #Patch Set 8 : Resync #Patch Set 9 : Bad merge #
Messages
Total messages: 54 (33 generated)
Description was changed from ========== Cleanup BUG= ========== to ========== Cleanup BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by pkasting@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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Description was changed from ========== Cleanup BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Misc. cleanup * Improve comments * Eliminate -webkit-transition that seems to do nothing * Group related CSS selectors near each other in preparation for combining/modifying them * Don't use RTL folder images on Mac (see https://cs.chromium.org/chromium/src/chrome/browser/resource_delegate_mac.mm?... ) * Use "x .y" instead of "x > * > .y" when the two will select the same elements, for brevity * Remove unnecessary size arg to getFaviconImageSet() (matches default value) * Don't use the name "scale_factor" for floats relating to scale, as "scale" and "scale factor" are distinct concepts. Use "scale" when that's what is meant. * Avoid unnecessary using directives. * Use a helper function to avoid repeating the same multi-part conditional * NULL -> nullptr * Improve function and variable names * Eliminate NOTREACHED() that can, in fact, be reached BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by pkasting@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...
pkasting@chromium.org changed reviewers: + dbeam@chromium.org, sky@chromium.org
sky: chrome/browser/ui/ OWNERS dbeam: rest Dan, I've marked a block in bmm.css as "Why did we have these?" I can't figure out why the URL field needs to always be nonempty, unless maybe the URL font could potentially be taller than the font used for the bookmark name, so without this, hovering a line could enlarge it? Can you figure out why we had these? I'd like to delete them but don't want to break anything.
The CQ bit was checked by pkasting@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
One question on your name change. https://codereview.chromium.org/2278423002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.h (right): https://codereview.chromium.org/2278423002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.h:51: float scale); Why do you prefer scale over scale_factor? I believe we use scale_factor in a number of places.
https://codereview.chromium.org/2278423002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.h (right): https://codereview.chromium.org/2278423002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.h:51: float scale); On 2016/08/26 16:25:46, sky wrote: > Why do you prefer scale over scale_factor? I believe we use scale_factor in a > number of places. ScaleFactor is an enum, while "scale" is a float value. The two have a correspondence; for example, https://cs.chromium.org/chromium/src/ui/base/resource/scale_factor.h provides GetScaleForScaleFactor() to convert the former into the latter. In the .cc file we use both concepts, so calling the latter "scale factor" as well is confusing.
The CQ bit was checked by pkasting@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 checked by pkasting@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...
BTW Dan, tell me if there's a reason to prefer "list > * > .label" or "list .label"; I was mostly confused about why some places used one and some the other, I don't know if one is actually more efficient.
On 2016/08/26 20:14:50, Peter Kasting wrote: > BTW Dan, tell me if there's a reason to prefer "list > * > .label" or "list > .label"; I was mostly confused about why some places used one and some the > other, I don't know if one is actually more efficient. (Given that, in this particular case, they'll end up with the same effect)
Fair enough, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
i would discourage your CSS changes unless there's a clear benefit. ">" means "direct descendant" or "child" combinator, whereas " " (space) means "indirect descendant" combinator. https://developer.mozilla.org/en-US/docs/Web/CSS/Child_selectors https://developer.mozilla.org/en-US/docs/Web/CSS/Descendant_selectors concretely: thing1 > * > thing2 means that <thing1> <div> <thing2><!-- just right --> will match, BUT <thing1> <thing2><!-- too shallow --> WONT match, nor will <thing1> <div> <span> <thing2><!-- too deep --> https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/css/bmm.css (left): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/css/bmm.css:50: list > * > .label { this denotes a rigid ancestry depth (i.e. only the first level of .label in a DAG). i would be careful about changing stuff like this unless you thoroughly understand what this does / have checked this code manually in many configurations. https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/css/bmm.css (right): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/css/bmm.css:52: display: inline-block; /* Makes the image start-align in RTL. */ /* Makes ^^ too many spaces https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.cc (right): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:35: GURL GetThemeURL(const std::string& path) { guidelines i've heard for new code using 3+ letter acronyms is to camel case, i.e. GetThemeUrl() https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:40: bool IsNewTabCSSPath(const std::string& path) { as well as IsNewTabCssPath() (if this were abbreviating "New Tab Page" as "NTP", IsNtpCssPath() would be easier to read than IsNTPCSSPath()) https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:44: return (path == kNewTabCSSPath) || (path == kIncognitoNewTabCSSPath); nit: the C++ style guide discourages () around return *expressions*. this isn't doing that, but I also don't think: return path == kNewTabCssPath || path == kIncognitoNewTabCssPath; is particularly hard to read. we also discourage extra punctuation in Chrome code (i.e. single-line conditionals) https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:134: const float max_scale = ui::GetScaleForScaleFactor( do we have to get max_scale in the event that resource_id == -1? it doesn't seem so. can we do an early out or at least only get the scale for scalefactor if we need to? most of our style guides encourage declaring/initializing values at late as possible https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:142: } else if ((GetMimeType(path) == "image/png") && (scale > max_scale)) { i don't really see the point to these () either, but you can leave them if it makes you really really happy inside https://codereview.chromium.org/2278423002/diff/80001/ui/webui/resources/css/... File ui/webui/resources/css/tree.css (left): https://codereview.chromium.org/2278423002/diff/80001/ui/webui/resources/css/... ui/webui/resources/css/tree.css:143: } why are you moving these? changing the order of the statements may have a functional affect (i.e. if there are 2 statements with similar specificity, the last one declared wins)
Have not updated yet, just replying. On 2016/08/30 01:27:29, Dan Beam wrote: > ">" means "direct descendant" or "child" combinator, whereas " " (space) means > "indirect descendant" combinator. Right, I understand that (or I wouldn't have had the confidence to make the changes I did). See reply below as to the purpose of this. https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/css/bmm.css (left): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/css/bmm.css:50: list > * > .label { On 2016/08/30 01:27:28, Dan Beam wrote: > this denotes a rigid ancestry depth (i.e. only the first level of .label in a > DAG). i would be careful about changing stuff like this unless you thoroughly > understand what this does / have checked this code manually in many > configurations. Since (AFAIK) we only add the label class to things that occur at this specific depth, the two have the same effect. I don't care much about using list > * > .label versus list .label. I'm happy to use either. But we should be consistent about using one or the other unless we actually need the difference. What I'm trying to fix is that this file is arbitrarily inconsistent; here we use the former, but the old code at line 76 used the latter. So let's pick one or the other and then always use it. If you think I should pick the other route, that's fine. https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.cc (right): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:44: return (path == kNewTabCSSPath) || (path == kIncognitoNewTabCSSPath); On 2016/08/30 01:27:29, Dan Beam wrote: > nit: the C++ style guide discourages () around return *expressions*. this isn't > doing that, but I also don't think: > > return path == kNewTabCssPath || path == kIncognitoNewTabCssPath; > > is particularly hard to read. There's nothing different about the rules for whether we should or shouldn't parenthesize subexprs inside, say, a return expression (as opposed to parenthesizing the whole expression), versus inside a conditional. So it doesn't really make sense to me why we'd remove them here but leave them below in the case where you wrote the comment about it "making me really happy". Either we should leave them both or remove them both. Chrome code has no consistent style about whether we put parens around subexprs always, sometimes (seemingly at random), or "only when it would change the order of evaluation". I find the middle one of those somewhat objectionable due to being arbitrarily inconsistent :) I personally do it always because I find it more readable, especially if things later begin taking multiple lines. I agree with you that it's not hard to read the alternative here; this is just about being consistent stylistically. > we also discourage extra punctuation in Chrome code (i.e. single-line > conditionals) I don't know what this sentence means. Avoiding "if (foo) return;" on a single-line, which is what I think you're talking about, doesn't seem to have anything to do with whether or not there's extra punctuation? https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:134: const float max_scale = ui::GetScaleForScaleFactor( On 2016/08/30 01:27:29, Dan Beam wrote: > do we have to get max_scale in the event that resource_id == -1? it doesn't > seem so. > > can we do an early out or at least only get the scale for scalefactor if we need > to? > > most of our style guides encourage declaring/initializing values at late as > possible Another CL I have LGTM on already that's waiting on this one makes it clearer why |max_scale| is declared as a temp here instead of inlined into the conditional below: https://codereview.chromium.org/2279373002/diff/20001/chrome/browser/ui/webui... We could recode that change to pull the resource_id == -1 check out separately, duplicating the code inside that condition's arm, but given that the calls here to get the max scale are actually very cheap, that seems worse overall. https://codereview.chromium.org/2278423002/diff/80001/ui/webui/resources/css/... File ui/webui/resources/css/tree.css (left): https://codereview.chromium.org/2278423002/diff/80001/ui/webui/resources/css/... ui/webui/resources/css/tree.css:143: } On 2016/08/30 01:27:29, Dan Beam wrote: > why are you moving these? changing the order of the statements may have a > functional affect (i.e. if there are 2 statements with similar specificity, the > last one declared wins) I'm trying to put them next to the similar RTL code in preparation for combining and replacing all those rules. (And I don't think the two selector blocks above have similar specificity to these, but if they did, we'd want them to win; so we'd want them declared last.)
The CQ bit was checked by pkasting@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...
Fixed the items below that were clearly wrong. Waiting for your reply on the other issues. Also still hoping for guidance on the "why did we have these" block in bmm.css. https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/css/bmm.css (right): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/css/bmm.css:52: display: inline-block; /* Makes the image start-align in RTL. */ On 2016/08/30 01:27:28, Dan Beam wrote: > /* Makes > ^^ > too many spaces Done. https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/theme_source.cc (right): https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:35: GURL GetThemeURL(const std::string& path) { On 2016/08/30 01:27:28, Dan Beam wrote: > guidelines i've heard for new code using 3+ letter acronyms is to camel case, > i.e. GetThemeUrl() Done. https://codereview.chromium.org/2278423002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/theme_source.cc:40: bool IsNewTabCSSPath(const std::string& path) { On 2016/08/30 01:27:29, Dan Beam wrote: > as well as IsNewTabCssPath() (if this were abbreviating "New Tab Page" as "NTP", > IsNtpCssPath() would be easier to read than IsNTPCSSPath()) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this seems fine to me, but why are one block that "take[s] up space" (in the file I commented on), but leaving another in the other .css file you're touching (same comment and ::after hacks in tree.css) https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (left): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:86: /* We need to ensure that even empty labels take up space */ ^ is this comment incorrect? https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (right): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:50: list .label { so are you absolutely sure that this is functionally the same selector? as in: the only .label elements are 2 levels below <list>?
On 2016/09/01 06:11:04, Dan Beam wrote: > this seems fine to me, but why are one block that "take[s] up space" (in the > file I commented on), but leaving another in the other .css file you're touching > (same comment and ::after hacks in tree.css) why are you removing**
On 2016/09/01 06:11:26, Dan Beam wrote: > On 2016/09/01 06:11:04, Dan Beam wrote: > > this seems fine to me, but why are one block that "take[s] up space" (in the > > file I commented on), but leaving another in the other .css file you're > touching > > (same comment and ::after hacks in tree.css) > > why are you removing** I didn't remove any of it -- I moved part of it up and hope to remove the other part but haven't done so yet. See replies below. https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (left): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:86: /* We need to ensure that even empty labels take up space */ On 2016/09/01 06:11:04, Dan Beam wrote: > ^ is this comment incorrect? You'll see that when I moved the .label portion of the block up, I expanded on the comment to say why we need to ensure this. For the .url part, I don't understand why this is needed, hence the "why did we have these?" question. From my initial email on this CR: "I can't figure out why the URL field needs to always be nonempty, unless maybe the URL font could potentially be taller than the font used for the bookmark name, so without this, hovering a line could enlarge it? Can you figure out why we had these? I'd like to delete them but don't want to break anything." https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (right): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:50: list .label { On 2016/09/01 06:11:04, Dan Beam wrote: > so are you absolutely sure that this is functionally the same selector? as in: > the only .label elements are 2 levels below <list>? A codesearch for "className = 'label" shows that only BookmarkListItem in bookmark_list.js applies here. AFAIK the bookmark manager's right pane only ever shows a list of BookmarkListItem, which would mean yes, these two selectors are the same. If, hypothetically, there was another place that .label elements could appear, then I think the old code would have been wrong to not do this for those as well. And the selectors below that assume .labels inside .folders only appear as children of lists are wrong too -- they should be "list .folder > .label" instead, right?
lgtm https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (left): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:86: /* We need to ensure that even empty labels take up space */ On 2016/09/01 07:13:31, Peter Kasting wrote: > On 2016/09/01 06:11:04, Dan Beam wrote: > > ^ is this comment incorrect? > > You'll see that when I moved the .label portion of the block up, I expanded on > the comment to say why we need to ensure this. > > For the .url part, I don't understand why this is needed, hence the "why did we > have these?" question. From my initial email on this CR: "I can't figure out > why the URL field needs to always be nonempty, unless maybe the URL font could > potentially be taller than the font used for the bookmark name, so without this, > hovering a line could enlarge it? Can you figure out why we had these? I'd > like to delete them but don't want to break anything." Acknowledged. https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (right): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:41: list > * > * { I suspect the selector below was list > * > .label so it shared a common prefix with this rule (so you could tell it was specializing it) https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:112: list .url:empty::after { delete this instead of commenting it out https://codereview.chromium.org/2278423002/diff/100001/ui/webui/resources/css... File ui/webui/resources/css/tree.css (right): https://codereview.chromium.org/2278423002/diff/100001/ui/webui/resources/css... ui/webui/resources/css/tree.css:147: /* We need to ensure that even empty labels take up space */ also necessary?
https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bookmark_manager/css/bmm.css (right): https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:41: list > * > * { On 2016/09/02 23:17:23, Dan Beam wrote: > I suspect the selector below was list > * > .label so it shared a common prefix > with this rule (so you could tell it was specializing it) Yeah, possibly. I thought about trying to keep the >s below for this reason, but then thought "these two selectors have no overlapping styles, so I guess it doesn't matter". https://codereview.chromium.org/2278423002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bookmark_manager/css/bmm.css:112: list .url:empty::after { On 2016/09/02 23:17:23, Dan Beam wrote: > delete this instead of commenting it out Yup https://codereview.chromium.org/2278423002/diff/100001/ui/webui/resources/css... File ui/webui/resources/css/tree.css (right): https://codereview.chromium.org/2278423002/diff/100001/ui/webui/resources/css... ui/webui/resources/css/tree.css:147: /* We need to ensure that even empty labels take up space */ On 2016/09/02 23:17:24, Dan Beam wrote: > also necessary? Don't know. I haven't really started hacking on this file in earnest yet. If I do and find this can go away, I can remove it later.
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2278423002/#ps120001 (title: "Remove dead rules")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2278423002/#ps140001 (title: "Resync")
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
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2278423002/#ps160001 (title: "Bad merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Misc. cleanup * Improve comments * Eliminate -webkit-transition that seems to do nothing * Group related CSS selectors near each other in preparation for combining/modifying them * Don't use RTL folder images on Mac (see https://cs.chromium.org/chromium/src/chrome/browser/resource_delegate_mac.mm?... ) * Use "x .y" instead of "x > * > .y" when the two will select the same elements, for brevity * Remove unnecessary size arg to getFaviconImageSet() (matches default value) * Don't use the name "scale_factor" for floats relating to scale, as "scale" and "scale factor" are distinct concepts. Use "scale" when that's what is meant. * Avoid unnecessary using directives. * Use a helper function to avoid repeating the same multi-part conditional * NULL -> nullptr * Improve function and variable names * Eliminate NOTREACHED() that can, in fact, be reached BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Misc. cleanup * Improve comments * Eliminate -webkit-transition that seems to do nothing * Group related CSS selectors near each other in preparation for combining/modifying them * Don't use RTL folder images on Mac (see https://cs.chromium.org/chromium/src/chrome/browser/resource_delegate_mac.mm?... ) * Use "x .y" instead of "x > * > .y" when the two will select the same elements, for brevity * Remove unnecessary size arg to getFaviconImageSet() (matches default value) * Don't use the name "scale_factor" for floats relating to scale, as "scale" and "scale factor" are distinct concepts. Use "scale" when that's what is meant. * Avoid unnecessary using directives. * Use a helper function to avoid repeating the same multi-part conditional * NULL -> nullptr * Improve function and variable names * Eliminate NOTREACHED() that can, in fact, be reached BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Misc. cleanup * Improve comments * Eliminate -webkit-transition that seems to do nothing * Group related CSS selectors near each other in preparation for combining/modifying them * Don't use RTL folder images on Mac (see https://cs.chromium.org/chromium/src/chrome/browser/resource_delegate_mac.mm?... ) * Use "x .y" instead of "x > * > .y" when the two will select the same elements, for brevity * Remove unnecessary size arg to getFaviconImageSet() (matches default value) * Don't use the name "scale_factor" for floats relating to scale, as "scale" and "scale factor" are distinct concepts. Use "scale" when that's what is meant. * Avoid unnecessary using directives. * Use a helper function to avoid repeating the same multi-part conditional * NULL -> nullptr * Improve function and variable names * Eliminate NOTREACHED() that can, in fact, be reached BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Misc. cleanup * Improve comments * Eliminate -webkit-transition that seems to do nothing * Group related CSS selectors near each other in preparation for combining/modifying them * Don't use RTL folder images on Mac (see https://cs.chromium.org/chromium/src/chrome/browser/resource_delegate_mac.mm?... ) * Use "x .y" instead of "x > * > .y" when the two will select the same elements, for brevity * Remove unnecessary size arg to getFaviconImageSet() (matches default value) * Don't use the name "scale_factor" for floats relating to scale, as "scale" and "scale factor" are distinct concepts. Use "scale" when that's what is meant. * Avoid unnecessary using directives. * Use a helper function to avoid repeating the same multi-part conditional * NULL -> nullptr * Improve function and variable names * Eliminate NOTREACHED() that can, in fact, be reached BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7f8dd6be8d00e838b371f904ddf2264172fdd826 Cr-Commit-Position: refs/heads/master@{#416425} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7f8dd6be8d00e838b371f904ddf2264172fdd826 Cr-Commit-Position: refs/heads/master@{#416425} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
