Fixed arguments order for AppendResultWithKey

Fixed arguments order for AppendResultWithKey (Google style guide dictates output params after input params).

Also, moved it out of RawDataPresenter.


BUG=157853
TEST=N/A (no functionality changes)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164547

[vabr (Chromium), 2012-10-25 15:39:24]

Hi again Dominic,

In the meantime, this CL is a low-priority nit.

Feel free to review it at any time you feel like it. :)

Vaclav

[battre, 2012-10-25 21:15:37]

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right):

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:106: //
Helpers shared with unit-tests.
can you please move this up and into the namespace extensions::subtle if you
want to make it a global function?

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:110: void
AppendResultWithKey(const char* key,
Do you want to rename this to AppendKeyValuePair?

[vabr (Chromium), 2012-10-26 08:18:15]

Hi Dominic,

Thanks for the comments, PTAL.

Vaclav

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right):

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:106: //
Helpers shared with unit-tests.
On 2012/10/25 21:15:37, battre wrote:
> can you please move this up and into the namespace extensions::subtle if you
> want to make it a global function?

Sure.

I forgot to mention why I would like this to not be part of RawDataPresenter:
As the original comment said, it had nothing to do with that class. For some
reason it was currently only used by RawDataParser-related code, but this could
change in the future.
Despite having written this function a couple of weeks ago, I was now confused
by its placement.

Also, I did not know about the "subtle" namespaces, but they seem to be used on
a couple of places. Just for my future reference, what exactly are those
supposed to contain?

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:110: void
AppendResultWithKey(const char* key,
On 2012/10/25 21:15:37, battre wrote:
> Do you want to rename this to AppendKeyValuePair?

Done.

[battre, 2012-10-26 09:05:37]

lgtm

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right):

https://codereview.chromium.org/11273036/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:106: //
Helpers shared with unit-tests.
On 2012/10/26 08:18:16, vabr (Chromium) wrote:
> On 2012/10/25 21:15:37, battre wrote:
> > can you please move this up and into the namespace extensions::subtle if you
> > want to make it a global function?
> 
> Sure.
> 
> I forgot to mention why I would like this to not be part of RawDataPresenter:
> As the original comment said, it had nothing to do with that class. For some
> reason it was currently only used by RawDataParser-related code, but this
could
> change in the future.
> Despite having written this function a couple of weeks ago, I was now confused
> by its placement.
> 
> Also, I did not know about the "subtle" namespaces, but they seem to be used
on
> a couple of places. Just for my future reference, what exactly are those
> supposed to contain?

It is just to prevent the pollution of the namespace from stuff that is an
implementation detail for one particular feature that should not be used by
other consumers.

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc
(right):

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:70:
&expected_list);
nit: do you want to format this like the lines above and below?

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:554:
&raw);
nit: do you want to format this line as the ones above and below?

[vabr (Chromium), 2012-10-26 09:25:06]

Thanks Dominic.

One nit implemented.
For the other, I believe to have reason to keep it as is (details in comments).

If you don't like it, please feel free to dispute.

If you are fine with that, you don't need to response.
Given that your last comment is green, I will send this to CQ later this
afternoon, unless I hear from you.

If I hear a complaint from you after this lands, I will happily address your
comments with a subsequent CL.

Cheers,
Vaclav

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc
(right):

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/upload_data_presenter_unittest.cc:70:
&expected_list);
On 2012/10/26 09:05:37, battre wrote:
> nit: do you want to format this like the lines above and below?

I've no strong opinion on this, so why not.

The support for the original layout was to make the middle call stand out,
because it is for a "file" block, whereas the others are for "bytes" blocks.
But I don't think it is that important to highlight it here.

Done.

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:554:
&raw);
On 2012/10/26 09:05:37, battre wrote:
> nit: do you want to format this line as the ones above and below?

Similarly to the other case, I thought it could be helpful to use the choice
given by the style guide to visually differentiate adding the "file" type block
from adding the "bytes" type block.

Compared to the previous case, there is much more data set up in this unit-test.
So I would like to keep this in, in hope that it makes reading the construction
of the data easier.

[battre, 2012-10-26 09:36:03]

lgtm

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/11273036/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:554:
&raw);
On 2012/10/26 09:25:07, vabr (Chromium) wrote:
> On 2012/10/26 09:05:37, battre wrote:
> > nit: do you want to format this line as the ones above and below?
> 
> Similarly to the other case, I thought it could be helpful to use the choice
> given by the style guide to visually differentiate adding the "file" type
block
> from adding the "bytes" type block.
> 
> Compared to the previous case, there is much more data set up in this
unit-test.
> So I would like to keep this in, in hope that it makes reading the
construction
> of the data easier.

Fine with me.

[commit-bot: I haz the power, 2012-10-26 09:48:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/11273036/15002

[commit-bot: I haz the power, 2012-10-27 12:23:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/11273036/15002

[commit-bot: I haz the power, 2012-10-27 18:20:15]

Change committed as 164547