Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1376243005:
Enabling included files when reloading local NTP with --local-ntp-reload. (Closed)
|
Created:
5 years, 2 months ago by tmartino Modified:
5 years ago CC:
chromium-reviews, David Black, dhollowa+watch_chromium.org, donnd+watch_chromium.org, dougw+watch_chromium.org, Jered, jfweitz+watch_chromium.org, kelvinp, kmadhusu+watch_chromium.org, melevin+watch_chromium.org, samarth+watch_chromium.org, skanuj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnabling included files when reloading local NTP with --local-ntp-reload.
Committed: https://crrev.com/5788cd432cc28372aa85a57cd7a66e6b6699db74
Cr-Commit-Position: refs/heads/master@{#359357}
Committed: https://crrev.com/9eafb7d4f9b985163c30354e1c7e34a9b447c275
Cr-Commit-Position: refs/heads/master@{#359650}
Committed: https://crrev.com/85dc4308873fb4cd712ad2bee0d71b7baa260f80
Cr-Commit-Position: refs/heads/master@{#362144}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressing fserb and mathp comments.: #
Total comments: 1
Patch Set 3 : Moving to local_files_ntp_source #Patch Set 4 : removing commented code: #
Total comments: 3
Patch Set 5 : Responding to mathp nits #Patch Set 6 : Fixing branch issues #Patch Set 7 : Changing string constant to char #Patch Set 8 : Adding ifdef guards to address iOS breakage #Patch Set 9 : Fixing ifdef formatting. #Patch Set 10 : Including build_config.h as necessary. #
Messages
Total messages: 43 (16 generated)
tmartino@chromium.org changed reviewers: + fserb@chromium.org, mathp@chromium.org
Whoops, sorry I forgot to actually hit publish on this.
https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:46: const std::string kInlineResouceRegex = "<include.*?src\\=[\"'](.+?)[\"'].*?>"; add a comment saying what you want to match with this. https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:181: void FlattenLocalInclude( can you add a comment here explaining the flow of callbacks and what we are trying to achieve? https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:196: // FlattenLocalInclude(callback, resource, remove those lines? https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:203: void CheckLocalIncludes(const content::URLDataSource::GotDataCallback& callback, couldn't this and FlattenLocalInclude be the same function? And maybe passing NULL to inlineResource?
https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:181: void FlattenLocalInclude( should be a private method and you wouldn't need to have a declaration and definition
Sorry for the delay! Cleaned up comments and moved methods to be private, as requested. See inline response re: code structure. https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/1376243005/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:203: void CheckLocalIncludes(const content::URLDataSource::GotDataCallback& callback, On 2015/10/06 at 15:53:17, fserb wrote: > couldn't this and FlattenLocalInclude be the same function? And maybe passing NULL to inlineResource? This doesn't really work, actually. We have two places where we need a function to be used as a callback: 1. Inside CheckLocalIncludesHelper, where we create "wrapper", which binds to the 3-arg function and provides a callback and a copy of the top-level resource. When the callback is invoked, a copy of the *inline* resource is passed as the last argument (per the spec for SendLocalFileResource). 2. Inside StartDataRequest, where we create "wrapper" which binds to the 2-arg function and provides only a callback. When the callback is invoked, a copy of the *top-level* resource is passed as the last argument (again, per the spec for SendLocalFileResource). Because SendLocalFileResource can only populate the last argument, and because we use it to load both the top-level and the inline resource, I don't really see a way we can condense these into the same function using a nullptr.
fserb@chromium.org changed required reviewers: + mathp@chromium.org
lgtm
https://codereview.chromium.org/1376243005/diff/20001/chrome/browser/search/l... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/1376243005/diff/20001/chrome/browser/search/l... chrome/browser/search/local_ntp_source.cc:186: void LocalNtpSource::CheckLocalIncludesHelper( these three functions don't really belong in this file. Please move to local_files_ntp_source.{cc,h}. SendLocalFileResource() should take care of all local inlining and call those functions as needed.
On 2015/10/19 at 19:59:01, mathp wrote: > https://codereview.chromium.org/1376243005/diff/20001/chrome/browser/search/l... > File chrome/browser/search/local_ntp_source.cc (right): > > https://codereview.chromium.org/1376243005/diff/20001/chrome/browser/search/l... > chrome/browser/search/local_ntp_source.cc:186: void LocalNtpSource::CheckLocalIncludesHelper( > these three functions don't really belong in this file. Please move to local_files_ntp_source.{cc,h}. > > SendLocalFileResource() should take care of all local inlining and call those functions as needed. Good call, this makes more sense. PTAL.
lgtm with nits https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... File chrome/browser/search/local_files_ntp_source.cc (right): https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... chrome/browser/search/local_files_ntp_source.cc:27: const std::string kInlineResouceRegex = "<include.*?src\\=[\"'](.+?)[\"'].*?>"; nit: Resource https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... chrome/browser/search/local_files_ntp_source.cc:66: std::string resource) { const std::string& resource ? https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... chrome/browser/search/local_files_ntp_source.cc:94: std::string topLevelResource, const std::string& ?
On 2015/10/29 at 20:15:19, mathp wrote: > lgtm with nits > > https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... > File chrome/browser/search/local_files_ntp_source.cc (right): > > https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... > chrome/browser/search/local_files_ntp_source.cc:27: const std::string kInlineResouceRegex = "<include.*?src\\=[\"'](.+?)[\"'].*?>"; > nit: Resource > Done. > https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... > chrome/browser/search/local_files_ntp_source.cc:66: std::string resource) { > const std::string& resource ? > Done. > https://codereview.chromium.org/1376243005/diff/60001/chrome/browser/search/l... > chrome/browser/search/local_files_ntp_source.cc:94: std::string topLevelResource, > const std::string& ? Can't be const or the call to RE2::Replace will fail. And base::Bind doesn't support nonconst references, so it's gotta be regular old std::string here.
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376243005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376243005/100001
The CQ bit was unchecked by tmartino@chromium.org
On 2015/10/30 19:35:19, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1376243005/100001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1376243005/100001 Ping?
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fserb@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/1376243005/#ps120001 (title: "Changing string constant to char")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376243005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376243005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1442793002/ by kelvinp@chromium.org. The reason for reverting is: Break iOS Device ninja Build Bot https://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/bui... Compilation failed as In file included from ../../chrome/browser/search/local_files_ntp_source.cc:19: ../../third_party/re2/re2/re2.h:186:10: fatal error: 're2/stringpiece.h' file not found #include "re2/stringpiece.h" which is added in this CL..
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5788cd432cc28372aa85a57cd7a66e6b6699db74 Cr-Commit-Position: refs/heads/master@{#359357}
Added ifdef guard which should fix the build issues that the trybots found on iOS. (Still not sure why those issues only popped up after I committed, and not on the initial try.) PTAL :)
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, fserb@chromium.org Link to the patchset: https://codereview.chromium.org/1376243005/#ps140001 (title: "Adding ifdef guards to address iOS breakage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376243005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376243005/140001
The CQ bit was unchecked by mathp@chromium.org
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, fserb@chromium.org Link to the patchset: https://codereview.chromium.org/1376243005/#ps160001 (title: "Fixing ifdef formatting.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376243005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376243005/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9eafb7d4f9b985163c30354e1c7e34a9b447c275 Cr-Commit-Position: refs/heads/master@{#359650}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1450533002/ by loyso@chromium.org. The reason for reverting is: CompileC /b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/Objects-normal/armv7/local_files_ntp_source.o browser/search/local_files_ntp_source.cc normal armv7 c++ com.apple.compilers.llvm.clang.1_0.compiler cd /b/build/slave/iOS_Device/build/src/chrome export LANG=en_US.US-ASCII export PATH="/Applications/Xcode7.0.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Applications/Xcode7.0.app/Contents/Developer/usr/bin:/Users/chrome-bot/slavebin:/b/depot_tools:/usr/local/git/bin:/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin" /b/build/slave/iOS_Device/build/src/chrome/../third_party/llvm-build/Release+Asserts/bin/clang -x c++ -arch armv7 -fmessage-length=0 -fdiagnostics-show-note-include-stack -fmacro-backtrace-limit=0 -std=c++11 -stdlib=libc++ -Wno-trigraphs -fno-exceptions -fno-rtti -Os -Werror -Wno-missing-field-initializers -Wno-missing-prototypes -Wno-return-type -Wno-non-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors -Wno-missing-braces -Wparentheses -Wswitch -Wno-unused-function -Wno-unused-label -Wno-unused-parameter -Wno-unused-variable -Wunused-value -Wno-empty-body -Wno-uninitialized -Wno-unknown-pragmas -Wno-shadow -Wno-four-char-constants -Wno-conversion -Wno-constant-conversion -Wno-int-conversion -Wno-bool-conversion -Wno-enum-conversion -Wno-shorten-64-to-32 -Wnewline-eof -Wno-c++11-extensions -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -DDISABLE_NACL -DCHROMIUM_BUILD -DCR_CLANG_REVISION=247874-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_CONFIGURATION_POLICY -DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED -DDISABLE_FTP_SUPPORT=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DUSE_SYSTEM_LIBXML -DMOJO_USE_SYSTEM_IMPL -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DNO_NSPR_10_SUPPORT -DNSPR_STATIC -DNSS_STATIC -DNSS_USE_STATIC_LIBS -DUSE_UTIL_DIRECTLY -DSK_SUPPORT_GPU=0 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_IGNORE_GL_TEXTURE_TARGET -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DUSE_LIBPCI=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DNS_BLOCK_ASSERTIONS=1 -D_FORTIFY_SOURCE=2 -isysroot /Applications/Xcode7.0.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.0.sdk -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -miphoneos-version-min=7.0 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -I/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/include -I/b/build/slave/iOS_Device/build/src/xcodebuild/DerivedSources/Release -I.. -I/b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/DerivedSources/Release -I../skia/config -I/Applications/Xcode7.0.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.0.sdk/usr/include/libxml2 -I../third_party/khronos -I../gpu -I/b/build/slave/iOS_Device/build/src/xcodebuild/DerivedSources/Release/protoc_out -I../third_party/protobuf -I../third_party/protobuf/src -I../third_party/dom_distiller_js/dist/proto_gen -I/b/build/slave/iOS_Device/build/src/xcodebuild/DerivedSources/Release/chrome -I/b/build/slave/iOS_Device/build/src/xcodebuild/DerivedSources/Release/components/strings -I../third_party/WebKit -I../third_party/nss/nspr/pr/include -I../third_party/nss/nspr/lib/ds -I../third_party/nss/nspr/lib/libc/include -I../third_party/nss/nss/lib/base -I../third_party/nss/nss/lib/certdb -I../third_party/nss/nss/lib/certhigh -I../third_party/nss/nss/lib/cryptohi -I../third_party/nss/nss/lib/dev -I../third_party/nss/nss/lib/freebl -I../third_party/nss/nss/lib/freebl/ecl -I../third_party/nss/nss/lib/nss -I../third_party/nss/nss/lib/pk11wrap -I../third_party/nss/nss/lib/pkcs7 -I../third_party/nss/nss/lib/pki -I../third_party/nss/nss/lib/smime -I../third_party/nss/nss/lib/softoken -I../third_party/nss/nss/lib/util -I../third_party/nss/nss/lib/ckfw/builtins -I../third_party/skia/include/core -I../third_party/skia/include/effects -I../third_party/skia/include/pdf -I../third_party/skia/include/gpu -I../third_party/skia/include/lazy -I../third_party/skia/include/pathops -I../third_party/skia/include/pipe -I../third_party/skia/include/ports -I../third_party/skia/include/utils -I../skia/ext -I../third_party/cacheinvalidation/overrides -I../third_party/cacheinvalidation/src -I../third_party/cacheinvalidation/google/cacheinvalidation -I../third_party/icu/source/i18n -I../third_party/icu/source/common -I../third_party/zlib -I/b/build/slave/iOS_Device/build/src/xcodebuild/DerivedSources/Release/ui/resources -I/b/build/slave/iOS_Device/build/src/xcodebuild/DerivedSources/Release/policy -I/b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/DerivedSources/armv7 -I/b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/DerivedSources -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wno-selector-type-mismatch -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-bitfield-width -Wexit-time-destructors -F/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos -Xclang -load -Xclang /b/build/slave/iOS_Device/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-templates -MMD -MT dependencies -MF /b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/Objects-normal/armv7/local_files_ntp_source.d --serialize-diagnostics /b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/Objects-normal/armv7/local_files_ntp_source.dia -c /b/build/slave/iOS_Device/build/src/chrome/browser/search/local_files_ntp_source.cc -o /b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/Objects-normal/armv7/local_files_ntp_source.o In file included from /b/build/slave/iOS_Device/build/src/chrome/browser/search/local_files_ntp_source.cc:21: ../third_party/re2/re2/re2.h:186:10: fatal error: 're2/stringpiece.h' file not found #include "re2/stringpiece.h" ^ 1 error generated. ** BUILD FAILED ** The following build commands failed: CompileC /b/build/slave/iOS_Device/build/src/xcodebuild/chrome.build/Release-iphoneos/browser.build/Objects-normal/armv7/local_files_ntp_source.o browser/search/local_files_ntp_source.cc normal armv7 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) .
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376243005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376243005/180001
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 tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, fserb@chromium.org Link to the patchset: https://codereview.chromium.org/1376243005/#ps180001 (title: "Including build_config.h as necessary.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376243005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376243005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Enabling included files when reloading local NTP with --local-ntp-reload. Committed: https://crrev.com/5788cd432cc28372aa85a57cd7a66e6b6699db74 Cr-Commit-Position: refs/heads/master@{#359357} Committed: https://crrev.com/9eafb7d4f9b985163c30354e1c7e34a9b447c275 Cr-Commit-Position: refs/heads/master@{#359650} ========== to ========== Enabling included files when reloading local NTP with --local-ntp-reload. Committed: https://crrev.com/5788cd432cc28372aa85a57cd7a66e6b6699db74 Cr-Commit-Position: refs/heads/master@{#359357} Committed: https://crrev.com/9eafb7d4f9b985163c30354e1c7e34a9b447c275 Cr-Commit-Position: refs/heads/master@{#359650} Committed: https://crrev.com/85dc4308873fb4cd712ad2bee0d71b7baa260f80 Cr-Commit-Position: refs/heads/master@{#362144} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/85dc4308873fb4cd712ad2bee0d71b7baa260f80 Cr-Commit-Position: refs/heads/master@{#362144} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
