Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1004253002:
Enable <webview>.executeScript outside of Apps and Extensions [2] (Closed)
|
Created:
5 years, 9 months ago by Xi Han Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable <webview>.executeScript outside of Apps and Extensions [2]
This patch enables javascript file injection for
<webview>.executeScript({file: ...}).
BUG=434081
Committed: https://crrev.com/961437079f62ecfb9a0847469a7c02ba1dda0c5d
Cr-Commit-Position: refs/heads/master@{#322623}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : nits #
Total comments: 2
Patch Set 3 : Fady's commments #
Total comments: 3
Patch Set 4 : nits #
Total comments: 4
Patch Set 5 : nits #Patch Set 6 : Use scoped_ptr. #Patch Set 7 : Clean up. #
Total comments: 6
Patch Set 8 : #Patch Set 9 : Add a test. #
Total comments: 9
Patch Set 10 : Devlin's comments. #
Total comments: 15
Patch Set 11 : Fix the memory leak. #Patch Set 12 : Antony's comments #
Total comments: 6
Patch Set 13 : Devlin's comments. #
Total comments: 11
Patch Set 14 : Another round of comments. #
Total comments: 2
Patch Set 15 : callback.Reset() #
Total comments: 4
Patch Set 16 : nits #Messages
Total messages: 58 (18 generated)
Patchset #1 (id:1) has been deleted
hanxi@chromium.org changed reviewers: + fsamuel@chromium.org
I will write a test ASAP.
Could you please pass this along to someone who understands the loading code better? I'll give it an OWNER rubberstamp at the end.
https://codereview.chromium.org/1004253002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1004253002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1802: bool ExecuteCodeInTabFunction::LoadFileForWebUI( Make this code the default implementation in ExecuteCodeFunction. https://codereview.chromium.org/1004253002/diff/20001/extensions/browser/api/... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/20001/extensions/browser/api/... extensions/browser/api/execute_code_function.h:41: const WebUILoadFileCallback& callback) = 0; Give this a default implementation here instead.
Hi alg@: Please review changes related to URLFetcher: - extensions/browser/api/guest_view/web_view/web_view_internal_api.h - extensions/browser/api/guest_view/web_view/web_view_internal_api.cc Thanks!
https://codereview.chromium.org/1004253002/diff/40001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/40001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:106: WebViewInternalExecuteCodeFunction* function_; use scoped_refptr
https://codereview.chromium.org/1004253002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1004253002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1802: bool ExecuteCodeInTabFunction::LoadFileForWebUI( On 2015/03/14 04:26:56, Fady Samuel wrote: > Make this code the default implementation in ExecuteCodeFunction. Done. https://codereview.chromium.org/1004253002/diff/20001/extensions/browser/api/... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/20001/extensions/browser/api/... extensions/browser/api/execute_code_function.h:41: const WebUILoadFileCallback& callback) = 0; On 2015/03/14 04:26:56, Fady Samuel wrote: > Give this a default implementation here instead. Done. https://codereview.chromium.org/1004253002/diff/40001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/40001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:106: WebViewInternalExecuteCodeFunction* function_; On 2015/03/16 14:24:31, Fady Samuel wrote: > use scoped_refptr Done.
https://codereview.chromium.org/1004253002/diff/60001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/60001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:107: const GURL& url_; GURL url_;
https://codereview.chromium.org/1004253002/diff/60001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/60001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:107: const GURL& url_; On 2015/03/16 15:16:26, Fady Samuel wrote: > GURL url_; Done.
hanxi@chromium.org changed reviewers: + agl@chromium.org
Hi Adam, please review changes related to URLFetcher in: - extensions/browser/api/guest_view/web_view/web_view_internal_api.h - extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
This looks like more than a rubber-stamp is needed from someone so I'm not the correct reviewer. You probably want to look at the log for the file that you're changing to find a suitable one.
https://codereview.chromium.org/1004253002/diff/60001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/60001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:225: WebUIURLFetcher* fetcher = new WebUIURLFetcher( Raw pointers look scary to me. Could you make this scoped_ptr, and then pass it along to fetchers_? https://codereview.chromium.org/1004253002/diff/80001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/80001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:102: DCHECK(result) << "Invalid fethcer setting."; typo: fetcher https://codereview.chromium.org/1004253002/diff/80001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:225: WebUIURLFetcher* fetcher = Make scoped_ptr?
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1004253002/diff/80001/extensions/browser/api/... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/80001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:102: DCHECK(result) << "Invalid fethcer setting."; On 2015/03/16 21:07:09, Fady Samuel wrote: > typo: fetcher Done. https://codereview.chromium.org/1004253002/diff/80001/extensions/browser/api/... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:225: WebUIURLFetcher* fetcher = On 2015/03/16 21:07:09, Fady Samuel wrote: > Make scoped_ptr? Since fetcher->start() is a async call, so we need to store fetcher in somewhere, and call fetcher.start(). ScopedVector can take care of raw pointer and delete them when its deconstructor is called. At the same time, we can still call fethcer->start(). If use scoped_ptr, then fethcer won't own the raw pointer anymore after passing the ownership to the fetchers.
hanxi@chromium.org changed reviewers: + asargent@chromium.org
asargent@chromium.org: Please review changes related to URLFetcher in: - extensions/browser/api/guest_view/web_view/web_view_internal_api.h - extensions/browser/api/guest_view/web_view/web_view_internal_api.cc Thanks!
Patchset #5 (id:120001) has been deleted
Hi Fady: as discussed offline, I change the creation of a raw pointer to a scoped_ptr, PTAL. Thanks!
lgtm but I'm not an expert on this code. A test would be very useful here.
FYI, I'm also not super knowledgeable about the code in question, so my questions might or might not make sense. It looks like Devlin reviewed the first of these changes for passing a script string - should he also have a look at this CL, since he has that context already? Or did you send to me just to shard the reviewing load? https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:227: if (!extension()) { should this test be "if (IsWebView())" instead of "if (!extension()"? https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:238: } Also, can you explain why this code is here in this class instead of in web_view_internal_api.{h,cc}? Presumably we don't want to allow this codepath when a non-extension script context calls chrome.tabs.executeScript, right? (since the implementation of that lives in ExecuteCodeInTabFunction which is also a subclass of ExecuteCodeFunction) https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... extensions/browser/api/execute_code_function.h:20: using WebUILoadFileCallback = base::Callback<void(bool, const std::string&)>; nit: please add a comment documenting the meaning of each of the 2 parameters to the callback (From context I'm assuming they are a flag for success and an error message, but it would be nice to have this mentioned explicitly to make the code easier to read)
Patchset #8 (id:200001) has been deleted
hanxi@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi Anotony: I thought you might be the right person to review the URLRequest part. That is ok, I will pass it to Devlin to see his opinions. Hi Devlin: please take a look the changes to load files from WebUI. Thanks! https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:227: if (!extension()) { On 2015/03/17 21:41:14, Antony Sargent wrote: > should this test be "if (IsWebView())" instead of "if (!extension()"? This is for webUI, which we have to use different ways to load javascript files. https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:238: } On 2015/03/17 21:41:14, Antony Sargent wrote: > Also, can you explain why this code is here in this class instead of in > web_view_internal_api.{h,cc}? > > Presumably we don't want to allow this codepath when a non-extension script > context calls chrome.tabs.executeScript, right? (since the implementation of > that lives in ExecuteCodeInTabFunction which is also a subclass of > ExecuteCodeFunction) That is fair. Move them back to web_view_internal_api.{h,cc}. https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/180001/extensions/browser/api... extensions/browser/api/execute_code_function.h:20: using WebUILoadFileCallback = base::Callback<void(bool, const std::string&)>; On 2015/03/17 21:41:14, Antony Sargent wrote: > nit: please add a comment documenting the meaning of each of the 2 parameters to > the callback (From context I'm assuming they are a flag for success and an error > message, but it would be nice to have this mentioned explicitly to make the code > easier to read) Done.
hanxi@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan: Please review the test added in -chrome/browser/ui/webui/* -chrome/test/data/webui/* Thanks!
Hi Xiyuan: Please review the test added in -chrome/browser/ui/webui/* -chrome/test/data/webui/* Thanks!
https://codereview.chromium.org/1004253002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui.cc (right): https://codereview.chromium.org/1004253002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui.cc:65: source->SetRequestFilter(base::Bind(&HandleTestFileRequestCallback)); We should only add this when the code runs as test. Think we can detect test by using the following: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); const bool is_running_test = command_line->HasSwitch(::switches::kTestName) || command_line->HasSwitch(::switches::kTestType);
https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:222: is_success = LoadFileForWebUI( Can we abstract this and the logic to get a file for an extension? Then, this would just become something like: if (!details_->file.get()) return false; LoadFile(*details_->file, base::Bind(&ExecuteCodeFunction::OnDidLoadFile, this)); https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:96: function_->OnWebUIURLFetchComplete(this); This strikes me as very odd. You both call directly into the api function, and run a callback (which calls into the function). I like the idea of the callback (since it abstracts out knowledge of the rest of the goings-on), but let's make it have just one callback, and not need the function at all. https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:236: fetchers_.erase(std::find(fetchers_.begin(), fetchers_.end(), fetcher)); When do we ever have multiple fetchers? Can't you only call a function with a single file?
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
Patchset #10 (id:300001) has been deleted
https://codereview.chromium.org/1004253002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/inline_login_ui.cc (right): https://codereview.chromium.org/1004253002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/inline_login_ui.cc:65: source->SetRequestFilter(base::Bind(&HandleTestFileRequestCallback)); On 2015/03/18 16:11:04, xiyuan wrote: > We should only add this when the code runs as test. > > Think we can detect test by using the following: > > base::CommandLine* command_line = > base::CommandLine::ForCurrentProcess(); > const bool is_running_test = > command_line->HasSwitch(::switches::kTestName) || > command_line->HasSwitch(::switches::kTestType); That is also what my concern is. Thanks! https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:222: is_success = LoadFileForWebUI( On 2015/03/18 16:52:27, Devlin wrote: > Can we abstract this and the logic to get a file for an extension? Then, this > would just become something like: > if (!details_->file.get()) > return false; > > LoadFile(*details_->file, base::Bind(&ExecuteCodeFunction::OnDidLoadFile, > this)); It is a good suggestion, but the logic of LoadFile for extensions is too complected to merge with the one with webUI. So we still keep two different callback functions here. But what we can do is abstract a LoadFile function which has the code for extensions in the base class, and move the webUI related logic to webview's subclass web_view_internal_api.cc{h}. https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:96: function_->OnWebUIURLFetchComplete(this); On 2015/03/18 16:52:27, Devlin wrote: > This strikes me as very odd. You both call directly into the api function, and > run a callback (which calls into the function). I like the idea of the callback > (since it abstracts out knowledge of the rest of the goings-on), but let's make > it have just one callback, and not need the function at all. Remove the call to the function here. https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:236: fetchers_.erase(std::find(fetchers_.begin(), fetchers_.end(), fetcher)); On 2015/03/18 16:52:27, Devlin wrote: > When do we ever have multiple fetchers? Can't you only call a function with a > single file? Hmmm, we need to store the fetcher in somewhere. That is fine, I will use a scoped_ptr to store it in the WebViewInternalExecuteCodeFunction.
webui/* LGTM
https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/240001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:236: fetchers_.erase(std::find(fetchers_.begin(), fetchers_.end(), fetcher)); On 2015/03/18 20:36:26, Xi Han wrote: > On 2015/03/18 16:52:27, Devlin wrote: > > When do we ever have multiple fetchers? Can't you only call a function with a > > single file? > > Hmmm, we need to store the fetcher in somewhere. That is fine, I will use a > scoped_ptr to store it in the WebViewInternalExecuteCodeFunction. Right; I wasn't saying we don't need to store it, just wondering about why we needed a vector (since it looks like there's only ever one). https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:94: callback_.Run(false, data); I think it'd be nice to rewrite this as: std::string data; bool result = false; if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS && fetcher_->GetResponseCode() == 200) { result = fetcher_->GetResponseAsString(&data); DCHECK(result); } callback_.Run(result, data); I think it's a little clearer, and also a few less lines of code. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:94: callback_.Run(false, data); I think it'd be nice to rewrite this as: std::string data; bool result = false; if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS && fetcher_->GetResponseCode() == 200) { result = fetcher_->GetResponseAsString(&data); DCHECK(result); } callback_.Run(result, data); I think it's a little clearer, and also a few less lines of code. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:105: const int render_process_id_; Is there a reason to store all these as member variables, instead of passing them into Start()? https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:227: fetcher_.reset(fetcher.release()); // Pass ownership to |fetcher_|. ... Why not just do fetcher_.reset(new WebUIUrlFetcher())? https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:238: if (!is_success) { inline is_success https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:239: SendResponse(false); Set an error? https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:253: SendResponse(false); Set errors.
I am still investigating the memory leak problem from asan try bots. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:94: callback_.Run(false, data); On 2015/03/19 15:45:35, Devlin wrote: > I think it'd be nice to rewrite this as: > > std::string data; > bool result = false; > if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS && > fetcher_->GetResponseCode() == 200) { > result = fetcher_->GetResponseAsString(&data); > DCHECK(result); > } > > callback_.Run(result, data); > > I think it's a little clearer, and also a few less lines of code. Updated. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:105: const int render_process_id_; On 2015/03/19 15:45:35, Devlin wrote: > Is there a reason to store all these as member variables, instead of passing > them into Start()? There is no special reason, just makes the start() more clear. I am ok with passing them in start(). Updated. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:227: fetcher_.reset(fetcher.release()); // Pass ownership to |fetcher_|. On 2015/03/19 15:45:35, Devlin wrote: > ... Why not just do fetcher_.reset(new WebUIUrlFetcher())? I also find that the old code causes memory leak. Updated. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:238: if (!is_success) { On 2015/03/19 15:45:35, Devlin wrote: > inline is_success Done. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:239: SendResponse(false); On 2015/03/19 15:45:35, Devlin wrote: > Set an error? Done. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:253: SendResponse(false); On 2015/03/19 15:45:36, Devlin wrote: > Set errors. Done.
https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:94: callback_.Run(false, data); On 2015/03/19 19:53:15, Xi Han wrote: > On 2015/03/19 15:45:35, Devlin wrote: > > I think it'd be nice to rewrite this as: > > > > std::string data; > > bool result = false; > > if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS && > > fetcher_->GetResponseCode() == 200) { > > result = fetcher_->GetResponseAsString(&data); > > DCHECK(result); > > } > > > > callback_.Run(result, data); > > > > I think it's a little clearer, and also a few less lines of code. > > Updated. Random drive-by: In the past we found bugs in extension code when we were explicitly checking for an http status code of 200, and considering it a failure when we got a perfectly valid and usable response from the cache with a status code of 304. Since this is a special URL it might not matter since it might not be something that is cacheable anyway, but as a general rule you should be wary of assuming that 200 is the only way a request can succeed and have usable results for you. Often just checking for URLRequestStatus::SUCCESS is sufficient.
The asan try bots turn green now, PTAL. Thanks. https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/320001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:94: callback_.Run(false, data); On 2015/03/19 20:24:38, Antony Sargent wrote: > On 2015/03/19 19:53:15, Xi Han wrote: > > On 2015/03/19 15:45:35, Devlin wrote: > > > I think it'd be nice to rewrite this as: > > > > > > std::string data; > > > bool result = false; > > > if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS && > > > fetcher_->GetResponseCode() == 200) { > > > result = fetcher_->GetResponseAsString(&data); > > > DCHECK(result); > > > } > > > > > > callback_.Run(result, data); > > > > > > I think it's a little clearer, and also a few less lines of code. > > > > Updated. > > Random drive-by: In the past we found bugs in extension code when we were > explicitly checking for an http status code of 200, and considering it a failure > when we got a perfectly valid and usable response from the cache with a status > code of 304. > > Since this is a special URL it might not matter since it might not be something > that is cacheable anyway, but as a general rule you should be wary of assuming > that 200 is the only way a request can succeed and have usable results for you. > Often just checking for URLRequestStatus::SUCCESS is sufficient. > That is a good point. Also I fount out URLRequestChromeJob::GetResponseCode()is always return ok(200), so it doesn't matter whether to check the GetResponseCode() here or not.
Sorry for the delay; thought I already responded to this one. https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... extensions/browser/api/execute_code_function.h:24: using WebUILoadFileCallback = base::Callback<void(bool, const std::string&)>; This doesn't belong here. https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:26: using net::URLFetcher; Nit: for the two places we use this, omit the "using" https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:240: void WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI( This doesn't really do anything different than DidLoadAndLocalizeFile(), so we should just use that.
Hi Devlin: PTAL, thanks. https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... extensions/browser/api/execute_code_function.h:24: using WebUILoadFileCallback = base::Callback<void(bool, const std::string&)>; On 2015/03/23 22:03:25, Devlin wrote: > This doesn't belong here. Move to WebViewInternalExecuteCodeFunction. https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:26: using net::URLFetcher; On 2015/03/23 22:03:25, Devlin wrote: > Nit: for the two places we use this, omit the "using" Actually, there are four places, but it is ok to omit this "using". https://codereview.chromium.org/1004253002/diff/360001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:240: void WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI( On 2015/03/23 22:03:25, Devlin wrote: > This doesn't really do anything different than DidLoadAndLocalizeFile(), so we > should just use that. They are slightly different with each other, since the DidLoadAndLocalizeFile() uses resource_ which can only be obtained from extensions, not webUI. So I have to abstract a common function to remove the code duplicate.
last few bits https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:114: void ExecuteCodeFunction::DidLoadFileForHost(const std::string& file, Why not just make DidLoadAndLocalizeFile() take in a resource_name, and add it on line 46? https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/execute_code_function.h:53: bool Execute(const std::string& code_string); This shouldn't be protected anymore, right? https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:98: GURL url_; There's still not really a point in persisting these (context_, url_) on the class, right? https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:226: base::Bind(&WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI, Just pass in DidLoadAndLocalizeFile(); there shouldn't be any harm in keeping the url_fetcher_ around until the function destructs (especially since you reset the real net::URLFetcher already on line 93) https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:228: if (is_success) inline is_success
Patchset #14 (id:400001) has been deleted
PTAL, thanks! https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/execute_code_function.cc:114: void ExecuteCodeFunction::DidLoadFileForHost(const std::string& file, On 2015/03/25 17:55:15, Devlin wrote: > Why not just make DidLoadAndLocalizeFile() take in a resource_name, and add it > on line 46? That is much simpler! Thanks! https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/execute_code_function.h:53: bool Execute(const std::string& code_string); On 2015/03/25 17:55:15, Devlin wrote: > This shouldn't be protected anymore, right? Good catch:) https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:98: GURL url_; On 2015/03/25 17:55:16, Devlin wrote: > There's still not really a point in persisting these (context_, url_) on the > class, right? Removed. https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:228: if (is_success) On 2015/03/25 17:55:15, Devlin wrote: > inline is_success Just confirmed what does "inline" mean here. Updated.
https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:226: base::Bind(&WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI, On 2015/03/25 17:55:15, Devlin wrote: > Just pass in DidLoadAndLocalizeFile(); there shouldn't be any harm in keeping > the url_fetcher_ around until the function destructs (especially since you reset > the real net::URLFetcher already on line 93) Missed this one?
https://codereview.chromium.org/1004253002/diff/420001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/420001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:93: callback_.Run(result, data); You'll also need to call callback_.reset() so that you un-refcount the WebViewInternalExtensionFunction (this is why the asan bots were unhappy). Right now, this is happening implicitly since you reset the url_fetcher_ in the callback, but that approach is a little dangerous (since it means that this object could be deleted while still inside this method).
Patchset #15 (id:440001) has been deleted
PTAL, thanks! https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/380001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:226: base::Bind(&WebViewInternalExecuteCodeFunction::DidLoadFileForWebUI, On 2015/03/25 20:57:49, Devlin wrote: > On 2015/03/25 17:55:15, Devlin wrote: > > Just pass in DidLoadAndLocalizeFile(); there shouldn't be any harm in keeping > > the url_fetcher_ around until the function destructs (especially since you > reset > > the real net::URLFetcher already on line 93) > > Missed this one? Done. https://codereview.chromium.org/1004253002/diff/420001/extensions/browser/api... File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc (right): https://codereview.chromium.org/1004253002/diff/420001/extensions/browser/api... extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:93: callback_.Run(result, data); On 2015/03/25 21:14:11, Devlin wrote: > You'll also need to call callback_.reset() so that you un-refcount the > WebViewInternalExtensionFunction (this is why the asan bots were unhappy). > Right now, this is happening implicitly since you reset the url_fetcher_ in the > callback, but that approach is a little dangerous (since it means that this > object could be deleted while still inside this method). Thanks for your help. Updated.
https://codereview.chromium.org/1004253002/diff/460001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/460001/extensions/browser/api... extensions/browser/api/execute_code_function.h:23: void DidLoadAndLocalizeFile(const std::string& file, Why is this public now? https://codereview.chromium.org/1004253002/diff/460001/extensions/browser/api... extensions/browser/api/execute_code_function.h:27: protected: I don't know why I didn't notice this before, but why do we have two 'protected' sections? The one on line 53 should be deleted. :)
Patchset #16 (id:480001) has been deleted
hanxi@chromium.org changed reviewers: - agl@chromium.org, asargent@chromium.org
Devlin: PTAL:) https://codereview.chromium.org/1004253002/diff/460001/extensions/browser/api... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/1004253002/diff/460001/extensions/browser/api... extensions/browser/api/execute_code_function.h:23: void DidLoadAndLocalizeFile(const std::string& file, On 2015/03/25 22:12:20, Devlin wrote: > Why is this public now? Oops, I bound &ExecuteCodeFunction::DidLoadAndLocalizeFile in web_view_internal_api.cc, but should bind &WebViewInternalExecuteCodeFunction::DidLoadAndLocalizeFile. Move to protected. https://codereview.chromium.org/1004253002/diff/460001/extensions/browser/api... extensions/browser/api/execute_code_function.h:27: protected: On 2015/03/25 22:12:20, Devlin wrote: > I don't know why I didn't notice this before, but why do we have two 'protected' > sections? The one on line 53 should be deleted. :) I didn't notice that either, removed.
lgtm
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1004253002/#ps500001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1004253002/500001
Message was sent while issue was closed.
Committed patchset #16 (id:500001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/961437079f62ecfb9a0847469a7c02ba1dda0c5d Cr-Commit-Position: refs/heads/master@{#322623} |
