Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21271002:
Added SkImageFilter serialization (Closed)
|
Created:
7 years, 4 months ago by sugoi1 Modified:
7 years, 4 months ago Reviewers:
jochen (gone - plz use gerrit), gab, senorblanco, piman, danakj, sugoi, palmer, Chris Evans CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded SkImageFilter serialization
BUG=164084
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217117
Patch Set 1 #
Total comments: 4
Patch Set 2 : Hid new ipc code behind flag and removed test changes #
Total comments: 2
Patch Set 3 : Unit test is back and enables the flag #
Total comments: 5
Patch Set 4 : Always Read/Write data, but ignore send/receive based on flag #
Total comments: 3
Patch Set 5 : Skia API changes #
Total comments: 2
Messages
Total messages: 48 (0 generated)
Here's a first version of the SkImageFilter uploaded to receive initial comments
Has there been a security review of the SkImageFilter for serialization to the browser process?
On 2013/07/30 18:00:06, danakj wrote: > Has there been a security review of the SkImageFilter for serialization to the browser process? No there hasn't been a security review yet. In the next steps, we definitely need to add safety checks on the Skia side and implement a fuzzer to test this (which may be a bit difficult since the whole thing will be sent as a large chunk of data). Just tell me what I need to do :)
On Tue, Jul 30, 2013 at 2:05 PM, <sugoi@chromium.org> wrote: > On 2013/07/30 18:00:06, danakj wrote: > >> Has there been a security review of the SkImageFilter for serialization >> to the >> > browser process? > > No there hasn't been a security review yet. In the next steps, we > definitely > need to add safety checks on the Skia side and implement a fuzzer to test > this > (which may be a bit difficult since the whole thing will be sent as a large > chunk of data). Just tell me what I need to do :) > Ok, maybe I'm wrong, but I feel like this would be the last step in the process, as once we start serializing and sending these to the browser, we'll be using them in the browser compositor. > > https://codereview.chromium.**org/21271002/<https://codereview.chromium.org/2... >
+palmer from security team
On 2013/07/30 18:06:31, danakj wrote: > On Tue, Jul 30, 2013 at 2:05 PM, <mailto:sugoi@chromium.org> wrote: > > Ok, maybe I'm wrong, but I feel like this would be the last step in the > process, as once we start serializing and sending these to the browser, > we'll be using them in the browser compositor. I agree, but we have a chicken and egg problem here, don't we ? How would I test the serialization code if it's not there ? I don't have to merge this right away, but I need to know that this code is good enough and that I won't have to re-write the serialization in an entirely different manner before I move on to other steps, so I kind of need to have this reviewed.
https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages.cc... content/common/cc_messages.cc:211: *r = skia::AdoptRef(buffer.readFlattenableT<SkImageFilter>()); Yes, obviously we can't land this until we know it's safe (otherwise it's opening a security hole). We can put this behind a flag though. https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages_un... File content/common/cc_messages_unittest.cc (right): https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages_un... content/common/cc_messages_unittest.cc:138: EXPECT_EQ(a->filter, b->filter); Given that this compares by pointer, I assume this doesn't pass, does it? Is there a way we can compare by value?
https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages.cc... content/common/cc_messages.cc:211: *r = skia::AdoptRef(buffer.readFlattenableT<SkImageFilter>()); On 2013/07/30 19:57:35, piman wrote: > Yes, obviously we can't land this until we know it's safe (otherwise it's opening a security hole). > > We can put this behind a flag though. All right, if putting it behind a flag is an acceptable option and isn't too complicated in this case (it shouldn't be), I'll do that. https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages_un... File content/common/cc_messages_unittest.cc (right): https://codereview.chromium.org/21271002/diff/1/content/common/cc_messages_un... content/common/cc_messages_unittest.cc:138: EXPECT_EQ(a->filter, b->filter); On 2013/07/30 19:57:35, piman wrote: > Given that this compares by pointer, I assume this doesn't pass, does it? Is there a way we can compare by value? Oh yeah, missed that, I'll change the code.
Hid all the new IPC code behind a temporary flag, which will be removed once the code is deemed secure enough. I also remove the unit test changes, since they would fail without the flag. I'll bring them back when the flag gets removed.
On 2013/07/31 15:31:23, sugoi1 wrote: > Hid all the new IPC code behind a temporary flag, which will be removed once the > code is deemed secure enough. SGTM, but will leave for other reviewers. > I also remove the unit test changes, since they would fail without the flag. > I'll bring them back when the flag gets removed. Is there no way to set the flag from the unit test? Maybe in its startup code?
On 2013/07/31 17:08:00, Stephen White wrote: > Is there no way to set the flag from the unit test? Maybe in its startup code? Done. I hope I did it the right way.
https://codereview.chromium.org/21271002/diff/13001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/13001/content/common/cc_message... content/common/cc_messages.cc:196: return; As it is, this relies on the flag being identical on both sides of the IPC. It's probably true in this case, but there's always crazy reasons why they may not be. If they aren't, then the entire IPC gets confused and we'll fail badly. Could we instead, make the writer side write empty filter if the flag isn't set, and the reader side always read the data, but discard it (before parsing) if the flag isn't set? That way we fall back sanely (no filter) if either side doesn't have the flag.
On 2013/08/01 19:53:21, piman wrote: > https://codereview.chromium.org/21271002/diff/13001/content/common/cc_message... > File content/common/cc_messages.cc (right): > > https://codereview.chromium.org/21271002/diff/13001/content/common/cc_message... > content/common/cc_messages.cc:196: return; > As it is, this relies on the flag being identical on both sides of the IPC. It's > probably true in this case, but there's always crazy reasons why they may not > be. If they aren't, then the entire IPC gets confused and we'll fail badly. > > Could we instead, make the writer side write empty filter if the flag isn't set, > and the reader side always read the data, but discard it (before parsing) if the > flag isn't set? > > That way we fall back sanely (no filter) if either side doesn't have the flag. No problem. Will do.
https://codereview.chromium.org/21271002/diff/13001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/13001/content/common/cc_message... content/common/cc_messages.cc:196: return; On 2013/08/01 19:53:22, piman wrote: > As it is, this relies on the flag being identical on both sides of the IPC. It's > probably true in this case, but there's always crazy reasons why they may not > be. If they aren't, then the entire IPC gets confused and we'll fail badly. > > Could we instead, make the writer side write empty filter if the flag isn't set, > and the reader side always read the data, but discard it (before parsing) if the > flag isn't set? > > That way we fall back sanely (no filter) if either side doesn't have the flag. Done.
https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... content/common/cc_messages.cc:195: if (!command_line.HasSwitch(switches::kAllowFiltersOverIPC)) For security purposes, this check should be done in the browser process, right? https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... content/common/cc_messages.cc:211: if (!command_line.HasSwitch(switches::kAllowFiltersOverIPC)) { Same thing here.
https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... content/common/cc_messages.cc:195: if (!command_line.HasSwitch(switches::kAllowFiltersOverIPC)) On 2013/08/01 20:26:27, Chromium Palmer wrote: > For security purposes, this check should be done in the browser process, right? See my comments, I think it's ok, and preferable to not write the data that won't be read, but the authority on the decision is definitely browser-side. https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... content/common/cc_messages.cc:211: if (!command_line.HasSwitch(switches::kAllowFiltersOverIPC)) { On 2013/08/01 20:26:27, Chromium Palmer wrote: > Same thing here. FYI, that side is the one that runs in the browser process (or, more generally future-wise, the "privileged" process).
@palmer, @piman: so, is there anything else you want me to do for this cl ? I'm not sure if there's still an issue or not.
LGTM https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/17001/content/common/cc_message... content/common/cc_messages.cc:211: if (!command_line.HasSwitch(switches::kAllowFiltersOverIPC)) { > FYI, that side is the one that runs in the browser process (or, more generally > future-wise, the "privileged" process). As I (think I) understand it now: in Write, the unprivileged process optimizes by not sending the data unless it thinks it can. In Read, the privileged process actually enforces the policy. Is that right? I had a feeling that might be what was going on, and it makes sense. I felt a need to make sure since this is common code; usually, I look for privileged code in the browser and unprivileged code in the renderer. But, this is OK.
I should say, looks good to me as long as my understanding is correct.
On 2013/08/06 17:49:05, Chromium Palmer wrote: > I should say, looks good to me as long as my understanding is correct. Yes, your understanding is correct. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sugoi@chromium.org/21271002/28001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Adding reviewers (jochen@, cevans@) for missing owners approvals.
On 2013/08/06 18:35:48, sugoi1 wrote: > Adding reviewers (jochen@, cevans@) for missing owners approvals. When you add OWNERS reviewers, can you say what they should review? (piman@ should be reviewing all content/ here most likely)
On 2013/08/06 18:36:53, danakj wrote:
> On 2013/08/06 18:35:48, sugoi1 wrote:
> > Adding reviewers (jochen@, cevans@) for missing owners approvals.
>
> When you add OWNERS reviewers, can you say what they should review? (piman@
should be reviewing all content/ here most likely)
Right. So, according to OWNERS files, I need these files reviewed :
piman@:
chrome/browser/chromeos/login/chrome_restart_request.cc
jochen@:
content/browser/renderer_host/render_process_host_impl.cc
cevans@:
content/common/cc_messages.cc
content/common/cc_messages_unittest.cc
content/public/common/content_switches.cc
content/public/common/content_switches.h
LGTM - sorry, I missed the fact that you had uploaded a patch with my requested changes
On 2013/08/06 21:50:52, piman wrote: > LGTM - sorry, I missed the fact that you had uploaded a patch with my requested > changes I'm not sure about this CL. 1) It could use a better Description. It says what but not why: why do we need to serialize SkFilters? Why is it behind a flag? Is the intent to remove the flag at some stage? 2) Is this a message that is deserialized by the browser? The code for SkOrderedReadBuffer and SkImageFilter(SkFlattenableReadBuffer&) does not look very safe to me at all.
On Tue, Aug 6, 2013 at 3:13 PM, <cevans@chromium.org> wrote: > On 2013/08/06 21:50:52, piman wrote: > >> LGTM - sorry, I missed the fact that you had uploaded a patch with my >> > requested > >> changes >> > > I'm not sure about this CL. > Background: https://code.google.com/p/chromium/issues/detail?id=164084 > > 1) It could use a better Description. It says what but not why: why do we > need to serialize SkFilters? See bug. > Why is it behind a flag? Because it's not safe yet. > Is the intent to remove the flag at some stage? > Yes, once it's safe. > > 2) Is this a message that is deserialized by the browser? Yes > The code for > SkOrderedReadBuffer and SkImageFilter(**SkFlattenableReadBuffer&) does > not look > very safe to me at all. > It's not. But we need something in place so that we can exercise the code and start writing a fuzzer. Chicken-and-egg problem, hence the suggestion to put it behind a flag. > > https://codereview.chromium.**org/21271002/<https://codereview.chromium.org/2... >
On 2013/08/06 22:21:17, piman wrote: > On Tue, Aug 6, 2013 at 3:13 PM, <mailto:cevans@chromium.org> wrote: > > > On 2013/08/06 21:50:52, piman wrote: > > > >> LGTM - sorry, I missed the fact that you had uploaded a patch with my > >> > > requested > > > >> changes > >> > > > > I'm not sure about this CL. > > > > Background: https://code.google.com/p/chromium/issues/detail?id=164084 > > > > > 1) It could use a better Description. It says what but not why: why do we > > need > > to serialize SkFilters? > > > See bug. > > > > Why is it behind a flag? > > > Because it's not safe yet. > > > > Is the intent to remove the flag at some stage? > > > > Yes, once it's safe. > > > > > 2) Is this a message that is deserialized by the browser? > > > Yes > > > > The code for > > SkOrderedReadBuffer and SkImageFilter(**SkFlattenableReadBuffer&) does > > not look > > very safe to me at all. > > > > It's not. > > But we need something in place so that we can exercise the code and start > writing a fuzzer. Chicken-and-egg problem, hence the suggestion to put it > behind a flag. > > > > > > > https://codereview.chromium.**org/21271002/%3Chttps://codereview.chromium.org...> > > Ok thanks for the details @piman. LGTM FWIW, I think a code audit might be more manageable than a fuzzer.
On Tue, Aug 6, 2013 at 3:26 PM, <cevans@chromium.org> wrote: > On 2013/08/06 22:21:17, piman wrote: > > On Tue, Aug 6, 2013 at 3:13 PM, <mailto:cevans@chromium.org> wrote: >> > > > On 2013/08/06 21:50:52, piman wrote: >> > >> >> LGTM - sorry, I missed the fact that you had uploaded a patch with my >> >> >> > requested >> > >> >> changes >> >> >> > >> > I'm not sure about this CL. >> > >> > > Background: https://code.google.com/p/**chromium/issues/detail?id=** >> 164084 <https://code.google.com/p/chromium/issues/detail?id=164084> >> > > > >> > 1) It could use a better Description. It says what but not why: why do >> we >> > need >> > > to serialize SkFilters? >> > > > See bug. >> > > > > Why is it behind a flag? >> > > > Because it's not safe yet. >> > > > > Is the intent to remove the flag at some stage? >> > >> > > Yes, once it's safe. >> > > > >> > 2) Is this a message that is deserialized by the browser? >> > > > Yes >> > > > > The code for >> > SkOrderedReadBuffer and SkImageFilter(****SkFlattenableReadBuffer&) >> does >> >> > not look >> > very safe to me at all. >> > >> > > It's not. >> > > But we need something in place so that we can exercise the code and start >> writing a fuzzer. Chicken-and-egg problem, hence the suggestion to put it >> behind a flag. >> > > > > >> > >> > > https://codereview.chromium.****org/21271002/%3Chttps://codere** > view.chromium.org/21271002/ <http://codereview.chromium.org/21271002/>> > >> > >> > > Ok thanks for the details @piman. LGTM > > FWIW, I think a code audit might be more manageable than a fuzzer. > I think we want both. 1- audit and fix issues 2- have a fuzzer to protect us against regressions. > > https://codereview.chromium.**org/21271002/<https://codereview.chromium.org/2... >
https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/chrome_restart_request.cc (right): https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/chrome_restart_request.cc:71: ::switches::kAllowFiltersOverIPC, why do you add the flag here?
https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/chrome_restart_request.cc (right): https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/chrome_restart_request.cc:71: ::switches::kAllowFiltersOverIPC, On 2013/08/07 04:44:38, jochen wrote: > why do you add the flag here? I thought the flag should be persistent, even on restart, but maybe I misunderstood what this is doing ? Or maybe you think this is unnecessary/unwanted for some reason ?
On 2013/08/07 11:43:53, sugoi1 wrote: > https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/chrome_restart_request.cc (right): > > https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/chrome_restart_request.cc:71: > ::switches::kAllowFiltersOverIPC, > On 2013/08/07 04:44:38, jochen wrote: > > why do you add the flag here? > > I thought the flag should be persistent, even on restart, but maybe I > misunderstood what this is doing ? Or maybe you think this is > unnecessary/unwanted for some reason ? On Chrome OS, you can't specify a command line as user anyway.
https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/chrome_restart_request.cc (right): https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... chrome/browser/chromeos/login/chrome_restart_request.cc:71: ::switches::kAllowFiltersOverIPC, On 2013/08/07 04:44:38, jochen wrote: > why do you add the flag here? This forwards the flag if present to the renderer process for incognito sessions so that they behave the same way as logged in sessions on ChromeOS. See the comment below about keeping flags in sync between here and renderer process host impl. The same applies to this flag (maybe that comment should move up and be reworded)
On 2013/08/07 13:01:13, danakj wrote: > https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... > File chrome/browser/chromeos/login/chrome_restart_request.cc (right): > > https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/l... > chrome/browser/chromeos/login/chrome_restart_request.cc:71: > ::switches::kAllowFiltersOverIPC, > On 2013/08/07 04:44:38, jochen wrote: > > why do you add the flag here? > > This forwards the flag if present to the renderer process for incognito sessions > so that they behave the same way as logged in sessions on ChromeOS. See the > comment below about keeping flags in sync between here and renderer process host > impl. The same applies to this flag (maybe that comment should move up and be > reworded) I guess you mean guest mode? Anyway, how would the flag be set on ChromeOS in the first place?
From the command line for development purposes. Later from the session setup scripts that launch chrome when we want to turn it on for real. On Aug 7, 2013 9:02 AM, <jochen@chromium.org> wrote: > On 2013/08/07 13:01:13, danakj wrote: > > https://codereview.chromium.**org/21271002/diff/28001/** > chrome/browser/chromeos/login/**chrome_restart_request.cc<https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/login/chrome_restart_request.cc> > >> File chrome/browser/chromeos/login/**chrome_restart_request.cc (right): >> > > > https://codereview.chromium.**org/21271002/diff/28001/** > chrome/browser/chromeos/login/**chrome_restart_request.cc#**newcode71<https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/login/chrome_restart_request.cc#newcode71> > >> chrome/browser/chromeos/login/**chrome_restart_request.cc:71: >> ::switches::**kAllowFiltersOverIPC, >> On 2013/08/07 04:44:38, jochen wrote: >> > why do you add the flag here? >> > > This forwards the flag if present to the renderer process for incognito >> > sessions > >> so that they behave the same way as logged in sessions on ChromeOS. See >> the >> comment below about keeping flags in sync between here and renderer >> process >> > host > >> impl. The same applies to this flag (maybe that comment should move up >> and be >> reworded) >> > > I guess you mean guest mode? > > Anyway, how would the flag be set on ChromeOS in the first place? > > https://codereview.chromium.**org/21271002/<https://codereview.chromium.org/2... >
Yes, guest mode is what I meant there. On Aug 7, 2013 9:02 AM, <jochen@chromium.org> wrote: > On 2013/08/07 13:01:13, danakj wrote: > > https://codereview.chromium.**org/21271002/diff/28001/** > chrome/browser/chromeos/login/**chrome_restart_request.cc<https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/login/chrome_restart_request.cc> > >> File chrome/browser/chromeos/login/**chrome_restart_request.cc (right): >> > > > https://codereview.chromium.**org/21271002/diff/28001/** > chrome/browser/chromeos/login/**chrome_restart_request.cc#**newcode71<https://codereview.chromium.org/21271002/diff/28001/chrome/browser/chromeos/login/chrome_restart_request.cc#newcode71> > >> chrome/browser/chromeos/login/**chrome_restart_request.cc:71: >> ::switches::**kAllowFiltersOverIPC, >> On 2013/08/07 04:44:38, jochen wrote: >> > why do you add the flag here? >> > > This forwards the flag if present to the renderer process for incognito >> > sessions > >> so that they behave the same way as logged in sessions on ChromeOS. See >> the >> comment below about keeping flags in sync between here and renderer >> process >> > host > >> impl. The same applies to this flag (maybe that comment should move up >> and be >> reworded) >> > > I guess you mean guest mode? > > Anyway, how would the flag be set on ChromeOS in the first place? > > https://codereview.chromium.**org/21271002/<https://codereview.chromium.org/2... >
ok, lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sugoi@chromium.org/21271002/28001
Retried try job too often on linux_chromeos for step(s) ash_unittests, browser_tests, content_browsertests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
I was asked by the Skia folks to add an API for serialization, instead of exposing SkOrderedReadBuffer/SkOrderedWriteBuffer directly. The changes are small and only affect cc_messages.cc, where the code has been simplified to use the new API.
https://codereview.chromium.org/21271002/diff/82001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/21271002/diff/82001/content/common/cc_message... content/common/cc_messages.cc:213: *r = skia::AdoptRef(static_cast<SkImageFilter*>(flattenable)); Even after we make the deserialization safe in Skia terms, we need a way here to verify that the deserialized object is indeed a SkImageFilter. Otherwise a compromized renderer could serialize anything (e.g. a SkBitmap or whatever) and cause type confusion in the browser
On 2013/08/09 17:38:24, piman wrote: > https://codereview.chromium.org/21271002/diff/82001/content/common/cc_message... > File content/common/cc_messages.cc (right): > > https://codereview.chromium.org/21271002/diff/82001/content/common/cc_message... > content/common/cc_messages.cc:213: *r = > skia::AdoptRef(static_cast<SkImageFilter*>(flattenable)); > Even after we make the deserialization safe in Skia terms, we need a way here to > verify that the deserialized object is indeed a SkImageFilter. > Otherwise a compromized renderer could serialize anything (e.g. a SkBitmap or > whatever) and cause type confusion in the browser Agreed, I will most likely need to change the static_cast to a Skia function that returns a fully validated SkImageFilter (As soon as I know what all the possible required validations are). I'll make sure to involve palmer@ in the process.
On 2013/08/09 17:53:55, sugoi1 wrote: > Agreed, I will most likely need to change the static_cast to a Skia function > that returns a fully validated SkImageFilter (As soon as I know what all the > possible required validations are). I'll make sure to involve palmer@ in the > process. Many Skia classes already have a validate() function that is only compiled in Debug builds. Perhaps the SkImageFilter one could be made non-debug-only?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sugoi@chromium.org/21271002/82001
Message was sent while issue was closed.
Change committed as 217117
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/21271002/diff/82001/content/public/com... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/21271002/diff/82001/content/public/com... content/public/common/content_switches.cc:930: // Don't dump stuff here, follow the same order as the header. See this line. New switches should be added in alphabetical order with the rest of the switches in this file. Please fix this. Thanks, Gab
Message was sent while issue was closed.
On 2013/08/22 02:37:18, gab wrote: > https://chromiumcodereview.appspot.com/21271002/diff/82001/content/public/com... > File content/public/common/content_switches.cc (right): > > https://chromiumcodereview.appspot.com/21271002/diff/82001/content/public/com... > content/public/common/content_switches.cc:930: // Don't dump stuff here, follow > the same order as the header. > See this line. > > New switches should be added in alphabetical order with the rest of the switches > in this file. > > Please fix this. > > Thanks, > Gab Done. See https://codereview.chromium.org/22831029/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
