Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
300573002:
Add stream operators for URLPattern and URLPatternSet and unit test for URLPatternSet (Closed)
|
Created:
6 years, 7 months ago by aboxhall Modified:
6 years, 5 months ago Reviewers:
Jeffrey Yasskin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@permissions Visibility:
Public. |
DescriptionAdd stream operators for URLPattern and URLPatternSet and unit test for URLPatternSet
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282150
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review comments #
Total comments: 7
Patch Set 3 : Review comments part 2 #Patch Set 4 : rebase #
Messages
Total messages: 22 (0 generated)
Ping?
Sorry for the delay. https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern.cc:152: return out << '"' << url_pattern.GetAsString() << '"'; Similarly, #include <ostream> for this. https://codereview.chromium.org/300573002/diff/1/extensions/common/url_pattern.h File extensions/common/url_pattern.h (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern.h:241: std::ostream& operator<<(std::ostream& out, const URLPattern& url_pattern); You should probably add a #include <iosfwd> for this. I suspect it's compiling for you because <string> includes that, but that's not guaranteed. https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern_set.cc:109: copy(url_pattern_set.patterns().begin(), Heh. Nice use of the STL, but I think it'll wind up simpler just to write the for loop. https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern_set_unittest.cc:68: EXPECT_STREQ("{ }", stream.str().c_str()); You can use EXPECT_EQ("{ }", stream.str());
https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern.cc:152: return out << '"' << url_pattern.GetAsString() << '"'; On 2014/05/28 21:06:08, Jeffrey Yasskin wrote: > Similarly, #include <ostream> for this. Done. https://codereview.chromium.org/300573002/diff/1/extensions/common/url_pattern.h File extensions/common/url_pattern.h (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern.h:241: std::ostream& operator<<(std::ostream& out, const URLPattern& url_pattern); On 2014/05/28 21:06:08, Jeffrey Yasskin wrote: > You should probably add a #include <iosfwd> for this. I suspect it's compiling > for you because <string> includes that, but that's not guaranteed. Done. https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern_set.cc:109: copy(url_pattern_set.patterns().begin(), On 2014/05/28 21:06:08, Jeffrey Yasskin wrote: > Heh. Nice use of the STL, but I think it'll wind up simpler just to write the > for loop. Done (not much simpler actually!) https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/300573002/diff/1/extensions/common/url_patter... extensions/common/url_pattern_set_unittest.cc:68: EXPECT_STREQ("{ }", stream.str().c_str()); On 2014/05/28 21:06:08, Jeffrey Yasskin wrote: > You can use EXPECT_EQ("{ }", stream.str()); Done.
lgtm https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set.cc:104: out << "{ "; And <ostream> https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set.cc:106: using const_iterator = std::set<URLPattern>::const_iterator; This is C++11 so won't work everywhere. Use typedef instead, or avoid the alias since you're only using it once. https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... File extensions/common/url_pattern_set.h (right): https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set.h:103: std::ostream& operator<<(std::ostream& out, Also needs <iosfwd> https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set_unittest.cc:66: std::ostringstream stream; This should probably have an <sstream> include also.
https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set.cc:104: out << "{ "; On 2014/05/28 22:32:04, Jeffrey Yasskin wrote: > And <ostream> Done. https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set.cc:106: using const_iterator = std::set<URLPattern>::const_iterator; On 2014/05/28 22:32:04, Jeffrey Yasskin wrote: > This is C++11 so won't work everywhere. Use typedef instead, or avoid the alias > since you're only using it once. Done. https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... File extensions/common/url_pattern_set_unittest.cc (right): https://codereview.chromium.org/300573002/diff/20001/extensions/common/url_pa... extensions/common/url_pattern_set_unittest.cc:66: std::ostringstream stream; On 2014/05/28 22:32:04, Jeffrey Yasskin wrote: > This should probably have an <sstream> include also. Done.
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/300573002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13376)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13416)
On 2014/05/29 02:59:10, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_gpu on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13416) @jyasskin: Looks like something is going funky on Windows. Any tips for debugging?
On Thu, May 29, 2014 at 10:41 AM, <aboxhall@chromium.org> wrote: > On 2014/05/29 02:59:10, I haz the power (commit-bot) wrote: >> >> Try jobs failed on following builders: >> win_gpu on tryserver.chromium.gpu > > > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13416) > > @jyasskin: Looks like something is going funky on Windows. Any tips for > debugging? > > > https://codereview.chromium.org/300573002/ That's not even failing in your code. It's failing at https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... where it's trying to stream a std::wstring to a std::ostream, which doesn't work because the character types are different. This doesn't come up on Linux because our FilePath::StringType is std::string, with a matching character type. But if that's the problem, it shouldn't have worked in the first place on Windows: your patch shouldn't make a difference. Your patch could add ambiguities, but URLPatternSet doesn't have a constructor from wstring, so that shouldn't be it. So I'm stuck. :( You might be able to patch file_util.cc by using .AsUTF8Unsafe() instead of .value(), but I don't know how many files will have to change. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah, I was wondering whether it was doing some kind of type erasure (?????) that might cause some kind of ambiguity. I have no idea. I guess I'll just hit try again and see whether things have un-broken themselves... On Thu, May 29, 2014 at 10:54 AM, Jeffrey Yasskin <jyasskin@chromium.org> wrote: > On Thu, May 29, 2014 at 10:41 AM, <aboxhall@chromium.org> wrote: > > On 2014/05/29 02:59:10, I haz the power (commit-bot) wrote: > >> > >> Try jobs failed on following builders: > >> win_gpu on tryserver.chromium.gpu > > > > > > ( > http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13416 > ) > > > > @jyasskin: Looks like something is going funky on Windows. Any tips for > > debugging? > > > > > > https://codereview.chromium.org/300573002/ > > That's not even failing in your code. It's failing at > > https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... > where it's trying to stream a std::wstring to a std::ostream, which > doesn't work because the character types are different. This doesn't > come up on Linux because our FilePath::StringType is std::string, with > a matching character type. But if that's the problem, it shouldn't > have worked in the first place on Windows: your patch shouldn't make a > difference. Your patch could add ambiguities, but URLPatternSet > doesn't have a constructor from wstring, so that shouldn't be it. So > I'm stuck. :( You might be able to patch file_util.cc by using > .AsUTF8Unsafe() instead of .value(), but I don't know how many files > will have to change. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, May 29, 2014 at 10:56 AM, Alice Boxhall <aboxhall@google.com> wrote: > Yeah, I was wondering whether it was doing some kind of type erasure (?????) > that might cause some kind of ambiguity. No, it's just a typedef: https://code.google.com/p/chromium/codesearch/#chromium/src/base/files/file_p... > I have no idea. I guess I'll just > hit try again and see whether things have un-broken themselves... > > > On Thu, May 29, 2014 at 10:54 AM, Jeffrey Yasskin <jyasskin@chromium.org> > wrote: >> >> On Thu, May 29, 2014 at 10:41 AM, <aboxhall@chromium.org> wrote: >> > On 2014/05/29 02:59:10, I haz the power (commit-bot) wrote: >> >> >> >> Try jobs failed on following builders: >> >> win_gpu on tryserver.chromium.gpu >> > >> > >> > >> > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13416) >> > >> > @jyasskin: Looks like something is going funky on Windows. Any tips for >> > debugging? >> > >> > >> > https://codereview.chromium.org/300573002/ >> >> That's not even failing in your code. It's failing at >> >> https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... >> where it's trying to stream a std::wstring to a std::ostream, which >> doesn't work because the character types are different. This doesn't >> come up on Linux because our FilePath::StringType is std::string, with >> a matching character type. But if that's the problem, it shouldn't >> have worked in the first place on Windows: your patch shouldn't make a >> difference. Your patch could add ambiguities, but URLPatternSet >> doesn't have a constructor from wstring, so that shouldn't be it. So >> I'm stuck. :( You might be able to patch file_util.cc by using >> .AsUTF8Unsafe() instead of .value(), but I don't know how many files >> will have to change. > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@jyasskin This is still failing even after a rebase to bring in your change. :(
On 2014/07/09 15:52:18, aboxhall wrote: > @jyasskin This is still failing even after a rebase to bring in your change. :( I think it's working now. The trybots use LKGR even if your client is synced past that.
The CQ bit was checked by aboxhall@chromium.org
Ah, makes sense! On Wed, Jul 9, 2014 at 12:25 PM, <jyasskin@chromium.org> wrote: > On 2014/07/09 15:52:18, aboxhall wrote: > >> @jyasskin This is still failing even after a rebase to bring in your >> change. >> > :( > > I think it's working now. The trybots use LKGR even if your client is > synced > past that. > > https://codereview.chromium.org/300573002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/300573002/60001
Message was sent while issue was closed.
Change committed as 282150 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
