Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10780013:
Add reverse URL handler for shortening uber URLs (Closed)
|
Created:
8 years, 5 months ago by Cem Kocagil Modified:
8 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, mihaip-chromium-reviews_chromium.org, browser-components-watch_chromium.org, Aaron Boodman Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd reverse URL handler for shortening uber URLs
BUG=120493
TEST=Open the settings page. The address bar should show "chrome://settings". Click the "extensions" button on the left. The address bar should show "chrome://extensions". Click the "settings" button on the left, "show advanced settings", "content settings". The address bar should show "chrome://settings/content".
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157576
Patch Set 1 #Patch Set 2 : fixed frame navigation issues #
Total comments: 4
Patch Set 3 : added tests #
Total comments: 5
Patch Set 4 : comments #
Total comments: 12
Patch Set 5 : comments 2 #
Total comments: 2
Patch Set 6 : comments 3 #
Total comments: 3
Patch Set 7 : comments 4 #
Total comments: 1
Patch Set 8 : browser test #
Total comments: 5
Patch Set 9 : rebased #Patch Set 10 : comments 5 #Patch Set 11 : d #Patch Set 12 : #Patch Set 13 : do not handle path urls #Patch Set 14 : fix tests #Patch Set 15 : rebased 2 #Patch Set 16 : #Patch Set 17 : fix tests 2 #Patch Set 18 : short uber #
Total comments: 4
Patch Set 19 : #
Total comments: 4
Patch Set 20 : bugfix, style fix #
Total comments: 3
Messages
Total messages: 65 (0 generated)
This patch shortens the URLs as needed, but for some reason pushState and friends don't change the short URLs.
It turns out that the URL handler pairs are doing some quite complex stuff, such as rewriting without handling. Refactor? Investigate moar.
https://chromiumcodereview.appspot.com/10780013/diff/5001/chrome/browser/chro... File chrome/browser/chrome_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/5001/chrome/browser/chro... chrome/browser/chrome_content_browser_client.cc:163: url_canon::Replacements<char> replacements; This addition here is to ensure that the reverse handler is run. If we don't handle it here, the reverse handler won't be called. https://chromiumcodereview.appspot.com/10780013/diff/5001/chrome/browser/chro... chrome/browser/chrome_content_browser_client.cc:204: if (!url Some of these checks are probably not necessary, I'll remove them later. https://chromiumcodereview.appspot.com/10780013/diff/5001/content/browser/web... File content/browser/web_contents/navigation_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/5001/content/browser/web... content/browser/web_contents/navigation_controller_impl.cc:879: GURL temp = params.url; The new navigation entries we created here defaulted to not update the virtual URL, which broke pushState handling after subframe navigations. This change checks whether we actually need to use virtual URLs and updates the navigation entry field accordingly.
I'll do a more thorough review in a bit, but here's some initial comments. First, you should try to find a way to write unit tests for this. https://chromiumcodereview.appspot.com/10780013/diff/5001/chrome/browser/chro... File chrome/browser/chrome_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/5001/chrome/browser/chro... chrome/browser/chrome_content_browser_client.cc:207: || url->host() != chrome::kChromeUIUberHost) This doesn't follow the Chromium coding style. See: http://www.chromium.org/developers/coding-style
Updated. Added a test, addressed the style issue.
Thanks. Here are some more comments. https://chromiumcodereview.appspot.com/10780013/diff/11001/chrome/browser/chr... File chrome/browser/chrome_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/11001/chrome/browser/chr... chrome/browser/chrome_content_browser_client.cc:203: bool HandleWebUIReverse(GURL* url, content::BrowserContext* browser_context) { This needs a comment explaining what it does, including what the params are and what it returns. Also, it seems like this isn't using browser_context, so get rid of the var. https://chromiumcodereview.appspot.com/10780013/diff/11001/chrome/browser/chr... chrome/browser/chrome_content_browser_client.cc:205: || url->host() != chrome::kChromeUIUberHost) This doesn't follow Chromium style. Put the || on the previous line. Also, you have an extra space before the || on the previous line. https://chromiumcodereview.appspot.com/10780013/diff/11001/chrome/browser/chr... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/11001/chrome/browser/chr... chrome/browser/chrome_content_browser_client_unittest.cc:11: using chrome::ChromeContentBrowserClient; Just put this test in the chrome namespace and get rid of the using line. https://chromiumcodereview.appspot.com/10780013/diff/11001/chrome/browser/chr... chrome/browser/chrome_content_browser_client_unittest.cc:13: class ChromeContentBrowserClientTest : public testing::Test { I don't think this is needed. https://chromiumcodereview.appspot.com/10780013/diff/11001/content/browser/we... File content/browser/web_contents/navigation_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/11001/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:881: BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary( Can you add a comment explaining what this is doing with examples of the case this handles that makes this necessary?
Updated. > This needs a comment explaining what it does, including what the > params are and what it returns. Also, it seems like this isn't using > browser_context, so get rid of the var. I can't remove the parameter even if it's unused because HandleWebUIReverse is a URLHandler which is defined in browser_url_handler.h. That file also explains the parameters.
Sorry for taking a while to review this. Here are some more comments your way. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:164: replacements.SetHost("chrome", url_parse::Component(0, 6)); Nit: Make a const std::string in the local scope and then use its .length() for the second arg to Component(). http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:165: std::string host = url->host(); Nit: Should be const. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:167: GURL chrome_url = url->ReplaceComponents(replacements); Nit: Maybe extract the above block to a separate function for better readability. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:173: return true; Add a comment why this doesn't need to set *url even though it returns true. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:212: std::string old_path = url->path(); Nit: Should be const. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:214: int separator = old_path.find('/', 1); Nit: Should be const. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:218: new_host = old_path.substr(1); What if old_path is an empty string? (Also, does find('/', 1) work in that case? http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:225: replacements.ClearHost(); Is this needed given that you set it immediately after? http://codereview.chromium.org/10780013/diff/15001/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.cc (right): http://codereview.chromium.org/10780013/diff/15001/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:885: new_entry->set_update_virtual_url_with_url(reverse_on_redirect);*/ What's up with the "*/" here?
Updated. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:167: GURL chrome_url = url->ReplaceComponents(replacements); On 2012/08/07 14:59:33, Alexei Svitkine wrote: > Nit: Maybe extract the above block to a separate function for better > readability. Good idea. I went further and added two functions: AddUberHost and RemoveUberHost. http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:173: return true; On 2012/08/07 14:59:33, Alexei Svitkine wrote: > Add a comment why this doesn't need to set *url even though it returns true. The comment right above that block explains why it returns true (so the reverse handler will be called). http://codereview.chromium.org/10780013/diff/15001/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:218: new_host = old_path.substr(1); On 2012/08/07 14:59:33, Alexei Svitkine wrote: > What if old_path is an empty string? (Also, does find('/', 1) work in that case? It should work when old_path is empty. I checked the C++ spec for the behavior of string::find where pos >= length and it says it should return npos.
Thanks! Getting close now. Do your TEST= instructions encompass the entirety of the behavior this is changing? For example, is the change to navigation_controller_impl.cc covered by the TEST= instructions, or are more actions required to trigger the replaceState calls on the uber page? If so, please expand the TEST= instructions accordingly. http://codereview.chromium.org/10780013/diff/16005/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/16005/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:186: new_host = old_path.substr(1); Even if find() is okay when |old_path| is empty, this substr() won't be. Please handle this case better. http://codereview.chromium.org/10780013/diff/16005/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:197: *url = url->ReplaceComponents(replacements); Can you extract this block into a helper function, calling it something like ReplaceURLHostAndPath(), passing in two strings? Then, you can call it from both RemoveUberHost() and AddUberHost().
Updated. - Changed TEST= to test the NavigationControllerImpl change as well. - Fixed the substr issue (I should have checked the spec for substr too, my bad) - Added ReplaceURLHostAndPath
https://chromiumcodereview.appspot.com/10780013/diff/11003/chrome/browser/chr... File chrome/browser/chrome_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/11003/chrome/browser/chr... chrome/browser/chrome_content_browser_client.cc:161: GURL ReplaceURLHostAndPath(const GURL url, const std::string host, Pass these args by ref. Put the 2nd and 3rd args on separate lines and align them with the first arg. Add a 1-line comment before the function. https://chromiumcodereview.appspot.com/10780013/diff/11003/chrome/browser/chr... chrome/browser/chrome_content_browser_client.cc:179: // If url->host() is "chrome", it maps "foo://chrome/bar/" to "foo://bar/" Explain the return value in the comment. End the sentence with a ".". https://chromiumcodereview.appspot.com/10780013/diff/11003/chrome/browser/chr... chrome/browser/chrome_content_browser_client.cc:1599: &HandleWebUIReverse); Nit: This can now fit on the previous line.
Updated.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/10780013/17003
Presubmit check for 10780013-17003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
content/browser/web_contents
You added one or more #includes that violate checkdeps rules.
chrome/browser/chrome_content_browser_client_unittest.cc
Illegal include: "content/browser/browser_url_handler_impl.h"
Because of no rule applying.
Presubmit checks took 1.4s to calculate.
-avi +ben, jam, sky for owners I'm not very familiar with the testing framework, so I need a bit of help. I want to test how ChromeContentBrowserClient uses BrowserURLHandler, but the implementation for BrowserURLHandler resides in content/. Do I need to write and expose a function that creates and returns a BrowserURLHandlerImpl in content/? If so, what would be an appropriate place for such function?
I swapped myself with Brett, who knows more about this.
if you run checkdeps, you'll see you got a layering violation. i would try to find another way of writing this test, in chrome (since that's what you want to test), without reaching into internal implementation headers of content. so maybe use ChromeRenderViewTest and navigate a few pages, or write a browser test etc http://codereview.chromium.org/10780013/diff/17003/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.cc (right): http://codereview.chromium.org/10780013/diff/17003/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:879: // For example, this allows uber page to update the virtual URL by calling nit: uber page should not be mentioned in content, since that's a (comment) layering violation
Updated. Uses a browser test now. Removed the comment in content/ mentioning uber. CQing for checkdeps.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/10780013/9004
Presubmit check for 10780013-9004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
content/browser/web_contents
Presubmit checks took 1.1s to calculate.
Could anyone review the new test? (Mailing again in case my last mail got buried under commit-bot mails)
I've started some try jobs for you; it seems there's patch failure on the Mac try bot; can you check if you're all synced up? https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... File chrome/browser/chrome_content_browser_client_browsertest.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... chrome/browser/chrome_content_browser_client_browsertest.cc:14: #include <string> This should be above the other includes. https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... chrome/browser/chrome_content_browser_client_browsertest.cc:16: using content::NavigationEntry; Put the test into the content namespace and removing the using declaration. https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... chrome/browser/chrome_content_browser_client_browsertest.cc:20: NavigationEntry* GetLastCommittedEntry() { Add a comment. https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... chrome/browser/chrome_content_browser_client_browsertest.cc:21: return chrome::GetTabContentsAt(browser(), 0)->web_contents() Is there a guarantee that nothing in the deref chain will be NULL?
Updated. https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... File chrome/browser/chrome_content_browser_client_browsertest.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/9004/chrome/browser/chro... chrome/browser/chrome_content_browser_client_browsertest.cc:21: return chrome::GetTabContentsAt(browser(), 0)->web_contents() On 2012/08/10 19:13:56, Alexei Svitkine wrote: > Is there a guarantee that nothing in the deref chain will be NULL? Before and after we navigate there should be one tab, so GetTabContentsAt() won't return NULL. This should own a WebContents, so web_contents() won't be NULL. GetController() returns a reference which can't be NULL. I think GetLastCommittedEntry() might return a NULL pointer if no entry is committed (not sure but this might be possible by adding a blank tab), so the test checks this value.
d
Your patch seems to be failing a bunch of tests. See the red win_rel, linux_rel and mac_rel links on the codereview page. You can click them to see which tests are failing. Please update the patch so that the tests don't fail. (You should be able to build and run the tests locally to debug the issues.)
Hey, whatever happened to this? Did you have a chance to look at the failing tests this patch was causing?
Yes, I have been away for a while, I'm now back and fixing the tests.
On 2012/09/12 14:40:30, Cem Kocagil wrote: > Yes, I have been away for a while, I'm now back and fixing the tests. Great! No worries, just wanted to make sure this didn't get lost in time and space. ;)
On 2012/09/12 14:42:23, Alexei Svitkine wrote: > On 2012/09/12 14:40:30, Cem Kocagil wrote: > > Yes, I have been away for a while, I'm now back and fixing the tests. > > Great! No worries, just wanted to make sure this didn't get lost in time and > space. ;) Okay, I think it should pass the tests now. Can you submit some tryjobs?
Try jobs submitted; will take a look at the changes later today.
Looks like 'update' step failed. You probably need to sync with latest code and merge/resolve conflicts and re-upload.
Rebased, could you submit tryjobs again?
Try jobs sent.
(Still update failures. Click through to the jobs to see the output.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/10780013/37001
Presubmit check for 10780013-37001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/browser/ui
content/browser/web_contents
chrome/browser/history
chrome/browser/extensions
Presubmit checks took 4.6s to calculate.
Okay, I couldn't get CQ to test it. Could you submit some tryjobs again?
On 2012/09/13 17:39:39, Cem Kocagil wrote: > Okay, I couldn't get CQ to test it. Could you submit some tryjobs again? Sent again.
short uber
On 2012/09/13 17:58:22, Alexei Svitkine wrote: > On 2012/09/13 17:39:39, Cem Kocagil wrote: > > Okay, I couldn't get CQ to test it. Could you submit some tryjobs again? > > Sent again. Sorry for the trouble, no patch I upload with git-cl works. Looks like I'll have to delete my repo. Anyway, I uploaded the same patch with svn, could you submit tryjobs again?
On 2012/09/13 18:58:00, Cem Kocagil wrote: > On 2012/09/13 17:58:22, Alexei Svitkine wrote: > > On 2012/09/13 17:39:39, Cem Kocagil wrote: > > > Okay, I couldn't get CQ to test it. Could you submit some tryjobs again? > > > > Sent again. > > Sorry for the trouble, no patch I upload with git-cl works. Looks like I'll have > to delete my repo. > > Anyway, I uploaded the same patch with svn, could you submit tryjobs again? Sorry for your troubles. I've sent the try jobs again.
Thanks! All bots except win_rel are green now. win_rel shows red but I checked and apparently that's due to flakes. I think you can review the rest of this patch now (just letting you know, nothing urgent).
+creis for navigation controller
Can you provide a better CL description, and provide a BUG= line referencing which bug this is fixing? https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... File content/browser/web_contents/navigation_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:929: // update the virtual URL when replaceState is called after a pushState. We were always doing this before, by using true on line 930. Why do we need to narrow it to only the cases that RewriteURLIfNecessary sets reverse_on_redirect to true? (I find this harder to follow and don't understand the extra complexity.) https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:930: GURL temp = params.url; nit: Please use a better variable name than temp.
Oh, gcl deleted my issue description, I'll write one again. https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... File content/browser/web_contents/navigation_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:929: // update the virtual URL when replaceState is called after a pushState. On 2012/09/13 22:10:09, creis wrote: > We were always doing this before, by using true on line 930. Why do we need to > narrow it to only the cases that RewriteURLIfNecessary sets reverse_on_redirect > to true? (I find this harder to follow and don't understand the extra > complexity.) Actually the only thing that is of importance to me here is the "set_update_virtual_url_with_url" call. Since I find whether the virtual URL needs to be updated, I thought I could set "update_virtual_url" to that value too (instead of always true). I can revert that line if you want, it doesn't affect the rest of this patch.
https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... File content/browser/web_contents/navigation_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10780013/diff/23011/content/browser/we... content/browser/web_contents/navigation_controller_impl.cc:929: // update the virtual URL when replaceState is called after a pushState. On 2012/09/13 22:40:03, Cem Kocagil wrote: > On 2012/09/13 22:10:09, creis wrote: > > We were always doing this before, by using true on line 930. Why do we need > to > > narrow it to only the cases that RewriteURLIfNecessary sets > reverse_on_redirect > > to true? (I find this harder to follow and don't understand the extra > > complexity.) > > Actually the only thing that is of importance to me here is the > "set_update_virtual_url_with_url" call. Since I find whether the virtual URL > needs to be updated, I thought I could set "update_virtual_url" to that value > too (instead of always true). > > I can revert that line if you want, it doesn't affect the rest of this patch. Oh, I didn't realize that was the important part. Feel free to leave the change to the update_virtual_url line. Perhaps this would be clearer if you renamed the bool to needs_update? (reverse_on_redirect isn't self-evident here.)
Updated, renamed temp and reverse_on_redirect.
Thanks. navigation_controller_impl.cc LGTM.
Still LGTM with a couple remaining nits. http://codereview.chromium.org/10780013/diff/26016/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/26016/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:211: if (url->host() != "") { Nit: Check !url->host().empty() instead. http://codereview.chromium.org/10780013/diff/26016/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator_browsertest.cc (right): http://codereview.chromium.org/10780013/diff/26016/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator_browsertest.cc:54: GURL ShortenUberURL(const GURL& url) { Nit. Add a comment.
Looks like we need reviewing and owners lgtms for a few files. Ben, could you check chrome_tests.gypi, browser_navigator_browsertest.cc, history_browsertest.cc, lazy_background_page_apitest.cc ?
lgtm
Alexei, please review the bugfix in chrome_content_browser_client.cc. If it still looks good, I'd like to commit this. http://codereview.chromium.org/10780013/diff/26016/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/26016/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:211: if (url->host() != "") { On 2012/09/14 14:15:27, Alexei Svitkine wrote: > Nit: Check !url->host().empty() instead. Done. http://codereview.chromium.org/10780013/diff/26016/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator_browsertest.cc (right): http://codereview.chromium.org/10780013/diff/26016/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator_browsertest.cc:54: GURL ShortenUberURL(const GURL& url) { On 2012/09/14 14:15:27, Alexei Svitkine wrote: > Nit. Add a comment. Done. http://codereview.chromium.org/10780013/diff/28018/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/28018/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:187: const std::string new_path = url.host() + url.path(); Fixed a bug here. This function didn't add the path before. It probably doesn't actually matter, but still.
http://codereview.chromium.org/10780013/diff/28018/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/28018/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:187: const std::string new_path = url.host() + url.path(); On 2012/09/17 18:28:40, Cem Kocagil wrote: > Fixed a bug here. This function didn't add the path before. It probably doesn't > actually matter, but still. Does this guarantee that there will be a slash ("/") between the two when path is non-empty?
http://codereview.chromium.org/10780013/diff/28018/chrome/browser/chrome_cont... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/10780013/diff/28018/chrome/browser/chrome_cont... chrome/browser/chrome_content_browser_client.cc:187: const std::string new_path = url.host() + url.path(); On 2012/09/17 18:34:26, Alexei Svitkine wrote: > On 2012/09/17 18:28:40, Cem Kocagil wrote: > > Fixed a bug here. This function didn't add the path before. It probably > doesn't > > actually matter, but still. > > Does this guarantee that there will be a slash ("/") between the two when path > is non-empty? Yes. GURL::path() says "Including first slash following host" as a comment in the header.
This is waiting for review (nothing urgent, just making sure my last reply is seen).
Oops, thought I replied already. Still LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/10780013/28018
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/10780013/28018
Retried try job too often for step(s) content_browsertests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cem.kocagil@gmail.com/10780013/28018
Change committed as 157576
This failed on the waterfall after landing (and reverted by the sheriffs): ChromeContentBrowserClientBrowserTest.UberURLHandler: chrome/browser/chrome_content_browser_client_browsertest.cc:46: Failure Value of: entry->GetURL() Actual: chrome://chrome/settings/ Expected: url2_long Which is: chrome://chrome/settings/content chrome/browser/chrome_content_browser_client_browsertest.cc:47: Failure Value of: entry->GetVirtualURL() Actual: chrome://settings/ Expected: url2_short Which is: chrome://settings/content Unfortunately, you'll have to create a new review with the patch and then fix that test and we can land it again. |
