Issue 1140053003: Refactoring: Moving the SafeJsonParser to its own component.

Issue 1140053003: Refactoring: Moving the SafeJsonParser to its own component. (Closed)

Created:
5 years, 7 months ago by Eran Messeri

Modified:
5 years, 6 months ago

Reviewers:
Robert Sesek, jschuh, jam

CC:
chromium-reviews, tapted, Matt Giuca, extensions-reviews_chromium.org, tfarina, pam+watch_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, Lei Zhang

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Refactoring: Moving the SafeJsonParser to its own component. So it can be used by other components, particularly parsing Certificate Transparency's Signed Tree Heads fetched from third-parties. BUG= Committed: https://crrev.com/b795155cfcf7583c9bc1d9527e07a5d3ee5742b3 Cr-Commit-Position: refs/heads/master@{#332573}

Patch Set 1 #

Patch Set 2 : Adding missing implementation file for IPC messages #

Patch Set 3 : Fixing test #

Patch Set 4 : Moving IPC handler to the component #

Patch Set 5 : Moving to namespace #

Total comments: 26

Patch Set 6 : Addressing review comments #

Patch Set 7 : Oops, forgot one comment. #

Patch Set 8 : Moving dependency around #

Total comments: 11

Patch Set 9 : Reverting most WebstoreInstallHelper changes #

Patch Set 10 : GN build fix attempt #

Patch Set 11 : #

Patch Set 12 : Localized string, build fixes #

Total comments: 10

Patch Set 13 : Build file fixes, addressing review comments #

Patch Set 14 : Removing export macro, duplicate android string #

Total comments: 6

Patch Set 15 : Catching up with WebstoreInstallHelper changes #

Created: 5 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+377 lines, -252 lines)			Patch
M	chrome/app/generated_resources.grd	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+0 lines, -3 lines	0 comments	Download
M	chrome/browser/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/DEPS	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/extensions/DEPS	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/management/chrome_management_api_delegate.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	3 chunks	+12 lines, -11 lines	0 comments	Download
M	chrome/browser/extensions/webstore_data_fetcher.cc	View	1 2 3 4	2 chunks	+6 lines, -7 lines	0 comments	Download
M	chrome/browser/extensions/webstore_install_helper.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/extensions/webstore_install_helper.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+2 lines, -2 lines	0 comments	Download
D	chrome/browser/safe_json_parser.h	View		1 chunk	+0 lines, -62 lines	0 comments	Download
D	chrome/browser/safe_json_parser.cc	View		1 chunk	+0 lines, -87 lines	0 comments	Download
M	chrome/browser/safe_json_parser_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/supervised_user/supervised_user_site_list.cc	View	1 2 3 4	2 chunks	+6 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/app_list/search/common/json_response_fetcher.cc	View	1 2 3 4	2 chunks	+7 lines, -9 lines	0 comments	Download
M	chrome/browser/web_resource/chrome_web_resource_service.cc	View	1 2 3 4	2 chunks	+4 lines, -3 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/chrome_utility.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+3 lines, -0 lines	0 comments	Download
M	chrome/common/chrome_utility_messages.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+0 lines, -17 lines	0 comments	Download
M	chrome/utility/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/utility/DEPS	View	9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/utility/chrome_content_utility_client.h	View	1 2 3	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/utility/chrome_content_utility_client.cc	View	1 2 3 4 5 6 7 8 9 10	4 chunks	+3 lines, -17 lines	0 comments	Download
A	chrome/utility/safe_json_parser_handler.h	View	1 2 3 4 5	1 chunk	+34 lines, -0 lines	0 comments	Download
A	chrome/utility/safe_json_parser_handler.cc	View	1 2 3	1 chunk	+18 lines, -0 lines	0 comments	Download
M	components/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/components.gyp	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/components_strings.grd	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+3 lines, -0 lines	0 comments	Download
A	components/safe_json_parser.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+49 lines, -0 lines	0 comments	Download
A	components/safe_json_parser/BUILD.gn	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+36 lines, -0 lines	0 comments	Download
A	components/safe_json_parser/DEPS	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+8 lines, -0 lines	0 comments	Download
A +	components/safe_json_parser/safe_json_parser.h	View	1 2 3 4 11 12 13 14	3 chunks	+7 lines, -3 lines	0 comments	Download
A +	components/safe_json_parser/safe_json_parser.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+24 lines, -14 lines	0 comments	Download
A	components/safe_json_parser/safe_json_parser_message_filter.h	View	1 2 3 4 5 11 12 13	1 chunk	+38 lines, -0 lines	0 comments	Download
A	components/safe_json_parser/safe_json_parser_message_filter.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+55 lines, -0 lines	0 comments	Download
A	components/safe_json_parser/safe_json_parser_messages.h	View	1 2 3 4 5	1 chunk	+37 lines, -0 lines	0 comments	Download
A +	components/safe_json_parser/safe_json_parser_messages.cc	View	1 2 3	1 chunk	+6 lines, -6 lines	0 comments	Download
M	ipc/ipc_message_start.h	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 26 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Eran Messeri

Robert, this CL is now ready for review, per your comments over chat. I intend ...

5 years, 7 months ago (2015-05-14 11:10:53 UTC) #2

Robert Sesek

https://codereview.chromium.org/1140053003/diff/80001/chrome/browser/extensions/webstore_install_helper.cc File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/1140053003/diff/80001/chrome/browser/extensions/webstore_install_helper.cc#newcode14 chrome/browser/extensions/webstore_install_helper.cc:14: #include "chrome/common/chrome_utility_messages.h" Unused. https://codereview.chromium.org/1140053003/diff/80001/chrome/browser/extensions/webstore_install_helper.cc#newcode19 chrome/browser/extensions/webstore_install_helper.cc:19: #include "content/public/browser/utility_process_host.h" Unused. https://codereview.chromium.org/1140053003/diff/80001/chrome/browser/extensions/webstore_install_helper.cc#newcode25 ...

5 years, 7 months ago (2015-05-14 20:12:34 UTC) #3

Robert Sesek

https://codereview.chromium.org/1140053003/diff/80001/components/safe_json_parser/safe_json_parser_message_filter.h File components/safe_json_parser/safe_json_parser_message_filter.h (right): https://codereview.chromium.org/1140053003/diff/80001/components/safe_json_parser/safe_json_parser_message_filter.h#newcode23 components/safe_json_parser/safe_json_parser_message_filter.h:23: }; DISALLOW_COPY_AND_ASSIGN

5 years, 7 months ago (2015-05-14 20:12:55 UTC) #4

Eran Messeri

Robert, thanks for the quick review! PTAL. Addressed all comments - the only one I ...

5 years, 7 months ago (2015-05-15 12:52:41 UTC) #5

Eran Messeri

John, add you as a reviewer per rsesek's suggestion, since it involves moving a class ...

5 years, 7 months ago (2015-05-18 16:32:13 UTC) #7

jam

https://codereview.chromium.org/1140053003/diff/140001/chrome/utility/DEPS File chrome/utility/DEPS (right): https://codereview.chromium.org/1140053003/diff/140001/chrome/utility/DEPS#newcode6 chrome/utility/DEPS:6: "+components/safe_json_parser", nit: order https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_parser/safe_json_parser.cc File components/safe_json_parser/safe_json_parser.cc (right): https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_parser/safe_json_parser.cc#newcode12 components/safe_json_parser/safe_json_parser.cc:12: ...

5 years, 7 months ago (2015-05-18 23:59:19 UTC) #8

Eran Messeri

Thanks for the quick review, PTAL. Note I undid the changes to the WebstoreInstallHelper as ...

5 years, 7 months ago (2015-05-19 20:49:56 UTC) #9

Thanks for the quick review, PTAL.
Note I undid the changes to the WebstoreInstallHelper as all tests using it
timed out - the callback for successful JSON parsing was never called. My
hypotheses that this was either due to incorrect WeakPtrFactory usage or
invoking callbacks on the wrong thread were both disproved.

https://codereview.chromium.org/1140053003/diff/140001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/1140053003/diff/140001/chrome/utility/DEPS#ne...
chrome/utility/DEPS:6: "+components/safe_json_parser",
On 2015/05/18 23:59:19, jam wrote:
> nit: order

Done.

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
File components/safe_json_parser/safe_json_parser.cc (right):

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
components/safe_json_parser/safe_json_parser.cc:12: #include
"ipc/ipc_message_macros.h"
On 2015/05/18 23:59:19, jam wrote:
> nit: this is an internal file, you shouldn't have to include it (i.e. a
> _messages.h file is enough)

Done.

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
components/safe_json_parser/safe_json_parser.cc:50:
host->SetName(base::ASCIIToUTF16(kJSONParserProcessName));
On 2015/05/18 23:59:19, jam wrote:
> seems like a regression here that you switch from translated to english name

Correct.
It does not seem to be trivial to get internatiolanized strings under
//components and ultimately this string is just used for a process name.
I argue that users savvy enough to dig into the list of processes running in
their browser would be OK with a non-internationalized name (but then again,
supporting non-latin characters in filenames was a rudimentary feature when I
started using computers...).
However if you can suggest an easy way to internationalize this string I'd be
happy to do it.

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
File components/safe_json_parser/safe_json_parser_message_filter.h (right):

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
components/safe_json_parser/safe_json_parser_message_filter.h:22: class
SafeJsonParserMessageFilter {
On 2015/05/18 23:59:19, jam wrote:
> why does this not derive from IPC::MessageFilter?
> 
> I don't understandy why chrome/utility/utility_message_handler.h exists in the
> first place instead of that just using IPC::MessageFilter interface

It doesn't derive from IPC::MessageFilter because it's not added as a filter to
any IPC ChannelProxy. It wouldn't conform to the interface's intentions if this
class derived from IPC::MessageFilter since it's used by the
UtilityMessageHandler which will only delegate OnMessageReceived. It would be
deceptive to derive from IPC::MessageFilter here since no other interface method
would be called.

Maybe vitalybuka@ can explain the reason for the existence of
UtilityMessageHandler.

Vitaly Buka (NO REVIEWS)

> It doesn't derive from IPC::MessageFilter because it's not added as a filter to > ...

5 years, 7 months ago (2015-05-20 05:43:25 UTC) #10

jam

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_parser/safe_json_parser.cc File components/safe_json_parser/safe_json_parser.cc (right): https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_parser/safe_json_parser.cc#newcode50 components/safe_json_parser/safe_json_parser.cc:50: host->SetName(base::ASCIIToUTF16(kJSONParserProcessName)); On 2015/05/19 20:49:56, Eran wrote: > On 2015/05/18 ...

5 years, 7 months ago (2015-05-20 15:38:31 UTC) #11

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
File components/safe_json_parser/safe_json_parser.cc (right):

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
components/safe_json_parser/safe_json_parser.cc:50:
host->SetName(base::ASCIIToUTF16(kJSONParserProcessName));
On 2015/05/19 20:49:56, Eran wrote:
> On 2015/05/18 23:59:19, jam wrote:
> > seems like a regression here that you switch from translated to english name
> 
> Correct.
> It does not seem to be trivial to get internatiolanized strings under
> //components and ultimately this string is just used for a process name.
> I argue that users savvy enough to dig into the list of processes running in
> their browser would be OK with a non-internationalized name (but then again,
> supporting non-latin characters in filenames was a rudimentary feature when I
> started using computers...).
> However if you can suggest an easy way to internationalize this string I'd be
> happy to do it.

Everywhere else that sets process names in the task manager appears to use a
localized string, so we should be consistent (otherwise the product looks
unpolished).

https://code.google.com/p/chromium/codesearch#search/&q=file:src/components%2...
shows examples of other components using translated strings.

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
File components/safe_json_parser/safe_json_parser_message_filter.h (right):

https://codereview.chromium.org/1140053003/diff/140001/components/safe_json_p...
components/safe_json_parser/safe_json_parser_message_filter.h:22: class
SafeJsonParserMessageFilter {
On 2015/05/19 20:49:56, Eran wrote:
> On 2015/05/18 23:59:19, jam wrote:
> > why does this not derive from IPC::MessageFilter?
> > 
> > I don't understandy why chrome/utility/utility_message_handler.h exists in
the
> > first place instead of that just using IPC::MessageFilter interface
> 
> It doesn't derive from IPC::MessageFilter because it's not added as a filter
to
> any IPC ChannelProxy. It wouldn't conform to the interface's intentions if
this
> class derived from IPC::MessageFilter since it's used by the
> UtilityMessageHandler which will only delegate OnMessageReceived. It would be
> deceptive to derive from IPC::MessageFilter here since no other interface
method
> would be called.
> 
> Maybe vitalybuka@ can explain the reason for the existence of
> UtilityMessageHandler.

ok.

Eran Messeri

jam, rsesek: PTAL. I've addressed John's comment about the localized string and seem to have ...

5 years, 7 months ago (2015-05-26 16:20:00 UTC) #12

Robert Sesek

Hm, I can't see an obvious reason why the webstore tests were failing. Could you ...

5 years, 7 months ago (2015-05-26 19:39:27 UTC) #13

jam

On 2015/05/26 16:20:00, Eran wrote: > jam, rsesek: PTAL. > I've addressed John's comment about ...

5 years, 7 months ago (2015-05-27 15:34:15 UTC) #14

Eran Messeri

rsesek, addressed all your comments, PTAL. Following our offline conversation yesterday I've added finer-grained targets ...

5 years, 6 months ago (2015-05-28 13:54:27 UTC) #15

Robert Sesek

LGTM w/ nits https://codereview.chromium.org/1140053003/diff/260001/chrome/browser/extensions/webstore_install_helper.cc File chrome/browser/extensions/webstore_install_helper.cc (left): https://codereview.chromium.org/1140053003/diff/260001/chrome/browser/extensions/webstore_install_helper.cc#oldcode87 chrome/browser/extensions/webstore_install_helper.cc:87: IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_ParseJSON_Succeeded, nit: preserve this indenting https://codereview.chromium.org/1140053003/diff/260001/components/safe_json_parser/safe_json_parser.cc ...

5 years, 6 months ago (2015-06-01 20:25:48 UTC) #16

Eran Messeri

@jschuh : Kindly review the changes to the IPC messages. I've moved the SafeJsonParser's messages ...

5 years, 6 months ago (2015-06-02 10:31:42 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140053003/280001

5 years, 6 months ago (2015-06-03 08:08:54 UTC) #22

commit-bot: I haz the power

Patchset 15 (id:??) landed as https://crrev.com/b795155cfcf7583c9bc1d9527e07a5d3ee5742b3 Cr-Commit-Position: refs/heads/master@{#332573}

5 years, 6 months ago (2015-06-03 08:40:28 UTC) #24

Lei Zhang

Drive by: Shouldn't every component have an OWNERS file? I filed https://crbug.com/500792 and don't have ...

5 years, 6 months ago (2015-06-16 03:39:32 UTC) #25

Robert Sesek

5 years, 6 months ago (2015-06-16 16:01:42 UTC) #26

Message was sent while issue was closed.

On 2015/06/16 03:39:32, Lei Zhang wrote:
> Drive by: Shouldn't every component have an OWNERS file? I filed
> https://crbug.com/500792 and don't have anyone to CC.

Yes there should be. Sorry for the oversight.
https://codereview.chromium.org/1188823003/

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages