Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916323004:
Expose GetLoggingFileName in ContentBrowserClient. (Closed)
DescriptionExpose GetLoggingFileName in ContentBrowserClient.
The V2 sandbox will need to get the path to the logging file name, so
this exposes the function to content/ through the client.
BUG=689306
Review-Url: https://codereview.chromium.org/2916323004
Cr-Commit-Position: refs/heads/master@{#479485}
Committed: https://chromium.googlesource.com/chromium/src/+/a67fad5ca2650a022181a7d06f502a94aa6f09f1
Patch Set 1 #Patch Set 2 : Simplify API #
Total comments: 6
Patch Set 3 : Cleanup per review #
Total comments: 4
Patch Set 4 : Move function order to match header #
Total comments: 2
Patch Set 5 : Clarify why function is exposed #Patch Set 6 : Rebase #Patch Set 7 : Fix namespace #Patch Set 8 : Expose GetLoggingFileName in ContentBrowserClient. #
Messages
Total messages: 48 (32 generated)
The CQ bit was checked by kerrnel@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...
kerrnel@chromium.org changed reviewers: + rsesek@chromium.org
PTAL. Thanks, Greg
The CQ bit was checked by kerrnel@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2916323004/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/2916323004/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client_unittest.cc:583: class GetLoggingFileTest : public testing::Test {}; ChromeContentBrowserClient... https://codereview.chromium.org/2916323004/diff/20001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2916323004/diff/20001/content/public/browser/... content/public/browser/content_browser_client.h:311: Here may be a better place to put the method, rather than just appending to the bottom. https://codereview.chromium.org/2916323004/diff/20001/content/public/browser/... content/public/browser/content_browser_client.h:819: // Returns the fully qualified path to the log file name, and returns whether It can't return both of these things.
The CQ bit was checked by kerrnel@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...
https://codereview.chromium.org/2916323004/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/2916323004/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client_unittest.cc:583: class GetLoggingFileTest : public testing::Test {}; On 2017/06/05 15:20:30, Robert Sesek wrote: > ChromeContentBrowserClient... Done. https://codereview.chromium.org/2916323004/diff/20001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2916323004/diff/20001/content/public/browser/... content/public/browser/content_browser_client.h:311: On 2017/06/05 15:20:30, Robert Sesek wrote: > Here may be a better place to put the method, rather than just appending to the > bottom. Done. https://codereview.chromium.org/2916323004/diff/20001/content/public/browser/... content/public/browser/content_browser_client.h:819: // Returns the fully qualified path to the log file name, and returns whether On 2017/06/05 15:20:30, Robert Sesek wrote: > It can't return both of these things. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ nits https://codereview.chromium.org/2916323004/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2916323004/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3619: base::FilePath ChromeContentBrowserClient::GetLoggingFileName() { Place in order with the .h https://codereview.chromium.org/2916323004/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/2916323004/diff/40001/content/public/browser/... content/public/browser/content_browser_client.cc:466: base::FilePath ContentBrowserClient::GetLoggingFileName() { Header order.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2916323004/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2916323004/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3619: base::FilePath ChromeContentBrowserClient::GetLoggingFileName() { On 2017/06/05 23:22:48, Robert Sesek wrote: > Place in order with the .h Done. https://codereview.chromium.org/2916323004/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/2916323004/diff/40001/content/public/browser/... content/public/browser/content_browser_client.cc:466: base::FilePath ContentBrowserClient::GetLoggingFileName() { On 2017/06/05 23:22:48, Robert Sesek wrote: > Header order. Done.
kerrnel@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/ Thanks, Greg
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: This issue passed the CQ dry run.
Seems ok, but we tend to ask for new public methods to have a caller as well to see how it's going to be used (and to avoid questions about whether an API is dead code if the next CL doesn't land right away). Is it possible to add the call within content to this CL, or link to the CL that adds it?
On 2017/06/06 16:34:19, Charlie Reis wrote: > Seems ok, but we tend to ask for new public methods to have a caller as well to > see how it's going to be used (and to avoid questions about whether an API is > dead code if the next CL doesn't land right away). Is it possible to add the > call within content to this CL, or link to the CL that adds it? I haven't added the caller because this is being used in a much larger CL and I want to keep the review under control. I think it's best then if we hold this until the other CL is ready for review, and we can make a dependency. Even without an LGTM, will you let me know if anything needs to change here? Thanks! - Greg
Sounds good. Basically looks good with the nit below, and we can revisit when the caller CL is ready. https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... content/public/browser/content_browser_client.h:323: // Returns the fully qualified path to the log file name, or an empty path. Might be worth mentioning why this is useful (e.g., so sandbox can make it available, presumably?). Otherwise people may think they should write to it directly.
The CQ bit was checked by kerrnel@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...
https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... content/public/browser/content_browser_client.h:323: // Returns the fully qualified path to the log file name, or an empty path. On 2017/06/06 17:14:10, Charlie Reis wrote: > Might be worth mentioning why this is useful (e.g., so sandbox can make it > available, presumably?). Otherwise people may think they should write to it > directly. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/06 17:18:17, Greg K wrote: > https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... > File content/public/browser/content_browser_client.h (right): > > https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... > content/public/browser/content_browser_client.h:323: // Returns the fully > qualified path to the log file name, or an empty path. > On 2017/06/06 17:14:10, Charlie Reis wrote: > > Might be worth mentioning why this is useful (e.g., so sandbox can make it > > available, presumably?). Otherwise people may think they should write to it > > directly. > > Done. Ok, here is the CL that uses the API: https://codereview.chromium.org/2931173003 The actual call is here at line 105: https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... That CL needs to be reviewed, but does it show you enough to LGTM this? Thanks, Greg
On 2017/06/09 22:12:39, Greg K wrote: > On 2017/06/06 17:18:17, Greg K wrote: > > > https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... > > File content/public/browser/content_browser_client.h (right): > > > > > https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... > > content/public/browser/content_browser_client.h:323: // Returns the fully > > qualified path to the log file name, or an empty path. > > On 2017/06/06 17:14:10, Charlie Reis wrote: > > > Might be worth mentioning why this is useful (e.g., so sandbox can make it > > > available, presumably?). Otherwise people may think they should write to it > > > directly. > > > > Done. > > Ok, here is the CL that uses the API: https://codereview.chromium.org/2931173003 > > The actual call is here at line 105: > https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... > > That CL needs to be reviewed, but does it show you enough to LGTM this? > > Thanks, > > Greg Sure. It doesn't look like there's a Chrome implementation of a delegate within ChildProcessLauncherHelper that could let us skip the ContentBrowserClient method, right? SandboxedProcessLauncherDelegate doesn't seem like it would fit that bill. (I ask because it would be nice to keep this within the sandbox code if possible.) Assuming there isn't, I'm ok with this approach. LGTM. If you can wait, though, it'd be better to land this after getting approval on the other CL as well, so we don't have a big gap between the two (e.g., in case the needs change during review).
On 2017/06/09 23:43:24, Charlie Reis wrote: > On 2017/06/09 22:12:39, Greg K wrote: > > On 2017/06/06 17:18:17, Greg K wrote: > > > > > > https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... > > > File content/public/browser/content_browser_client.h (right): > > > > > > > > > https://codereview.chromium.org/2916323004/diff/60001/content/public/browser/... > > > content/public/browser/content_browser_client.h:323: // Returns the fully > > > qualified path to the log file name, or an empty path. > > > On 2017/06/06 17:14:10, Charlie Reis wrote: > > > > Might be worth mentioning why this is useful (e.g., so sandbox can make it > > > > available, presumably?). Otherwise people may think they should write to > it > > > > directly. > > > > > > Done. > > > > Ok, here is the CL that uses the API: > https://codereview.chromium.org/2931173003 > > > > The actual call is here at line 105: > > > https://codereview.chromium.org/2931173003/diff/1/content/browser/child_proce... > > > > That CL needs to be reviewed, but does it show you enough to LGTM this? > > > > Thanks, > > > > Greg > > Sure. It doesn't look like there's a Chrome implementation of a delegate within > ChildProcessLauncherHelper that could let us skip the ContentBrowserClient > method, right? SandboxedProcessLauncherDelegate doesn't seem like it would fit > that bill. (I ask because it would be nice to keep this within the sandbox code > if possible.) > > Assuming there isn't, I'm ok with this approach. LGTM. If you can wait, > though, it'd be better to land this after getting approval on the other CL as > well, so we don't have a big gap between the two (e.g., in case the needs change > during review). Right I looked all over but the ContentClient seems to be the only suitable place for this. I will land this once the other CL is ready, sound good? Thanks, Greg
On 2017/06/12 18:35:03, Greg K wrote: > Right I looked all over but the ContentClient seems to be the only suitable > place for this. I will land this once the other CL is ready, sound good? > > Thanks, > > Greg Sounds good, thanks.
The CQ bit was checked by kerrnel@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: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@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: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2916323004/#ps140001 (title: "Expose GetLoggingFileName in ContentBrowserClient.")
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": 140001, "attempt_start_ts": 1497471141421280,
"parent_rev": "d7aa9b726f574cff3c44c068e49ec71ee6081449", "commit_rev":
"a67fad5ca2650a022181a7d06f502a94aa6f09f1"}
Message was sent while issue was closed.
Description was changed from ========== Expose GetLoggingFileName in ContentBrowserClient. The V2 sandbox will need to get the path to the logging file name, so this exposes the function to content/ through the client. BUG=689306 ========== to ========== Expose GetLoggingFileName in ContentBrowserClient. The V2 sandbox will need to get the path to the logging file name, so this exposes the function to content/ through the client. BUG=689306 Review-Url: https://codereview.chromium.org/2916323004 Cr-Commit-Position: refs/heads/master@{#479485} Committed: https://chromium.googlesource.com/chromium/src/+/a67fad5ca2650a022181a7d06f50... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a67fad5ca2650a022181a7d06f50... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
