Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2150513002:
Move web_js_test.h upstream (Closed)
|
Created:
4 years, 5 months ago by vabr (Chromium) Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove web_js_test.h upstream
This creates a copy of the downstream web_js_test.h upstream in
ios/web/public/test and puts it into the ios_web_test_support build target
(+equivalent in GN).
The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences:
(1) git cl format is required upstream, which changed indenting in a single location.
(2) namespace web was introduced to match the style of the target directory.
BUG=627775
Committed: https://crrev.com/b737caa2d98e0a27d283cd66ab777f9b42e042a2
Cr-Commit-Position: refs/heads/master@{#405217}
Patch Set 1 #Patch Set 2 : namespace web #Patch Set 3 : Squash 3 steps #
Total comments: 2
Patch Set 4 : include -> import #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 32 (21 generated)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move web_js_test.h upstream, part 1/3 This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into a temporary build target. This is the upstreaming CL 1 of 3. BUG=627775 ========== to ========== Move web_js_test.h upstream, part 1/3 This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into a temporary build target. The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. This is the upstreaming CL 1 of 3. BUG=627775 ==========
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move web_js_test.h upstream, part 1/3 This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into a temporary build target. The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. This is the upstreaming CL 1 of 3. BUG=627775 ========== to ========== Move web_js_test.h upstream This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into the ios_web_test_support build target (+equivalent in GN). The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. BUG=627775 ==========
vabr@chromium.org changed reviewers: + eugenebut@chromium.org, sdefresne@chromium.org
Dear reviewers! This is the upstream part of the plan in http://crbug.com/627775#c3. sdefresne@ -- could you please confirm that this is according to your suggestions? eugenebut@ -- could you please give this the OWNERS review, including checking if the destination is OK? The file is currently being used in downstream unittests, some of which are going to be upstreamed really soon. Cheers, Vaclav
lgtm https://codereview.chromium.org/2150513002/diff/40001/ios/web/public/test/web... File ios/web/public/test/web_js_test.h (right): https://codereview.chromium.org/2150513002/diff/40001/ios/web/public/test/web... ios/web/public/test/web_js_test.h:10: #include "base/mac/bundle_locations.h" nit: import here too
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review, comment addressed. Eugene, I'll wait for your review to confirm that the destination looks correct (and of course to see if you have any other comments). Cheers, Vaclav https://codereview.chromium.org/2150513002/diff/40001/ios/web/public/test/web... File ios/web/public/test/web_js_test.h (right): https://codereview.chromium.org/2150513002/diff/40001/ios/web/public/test/web... ios/web/public/test/web_js_test.h:10: #include "base/mac/bundle_locations.h" On 2016/07/13 12:50:00, sdefresne wrote: > nit: import here too Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_js_test.h (right): https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... ios/web/public/test/web_js_test.h:23: : java_script_paths_([java_script_paths retain]) {} NIT: Consider copying array since the input can be mutable. https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... ios/web/public/test/web_js_test.h:63: // Wait until main web injection has occured. Do we still need this "wait"? I would assume that web JS is injected immediately into WKWebView. https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... ios/web/public/test/web_js_test.h:72: WebTestT::EvaluateJavaScriptAsString([NSString NIT: EvaluateJavaScriptAsString is marked as deprecated. Can you replace this with |ExecuteJavaScript|? We plan to clean this up later, so if you prefer to leave the code as it is it's fine.
On 2016/07/13 15:22:27, Eugene But wrote: > lgtm > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > File ios/web/public/test/web_js_test.h (right): > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > ios/web/public/test/web_js_test.h:23: : java_script_paths_([java_script_paths > retain]) {} > NIT: Consider copying array since the input can be mutable. > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > ios/web/public/test/web_js_test.h:63: // Wait until main web injection has > occured. > Do we still need this "wait"? I would assume that web JS is injected immediately > into WKWebView. > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > ios/web/public/test/web_js_test.h:72: > WebTestT::EvaluateJavaScriptAsString([NSString > NIT: EvaluateJavaScriptAsString is marked as deprecated. Can you replace this > with |ExecuteJavaScript|? > > We plan to clean this up later, so if you prefer to leave the code as it is it's > fine. Thanks, Eugene! To be honest, I have not much experience with this particular template. If you are sure the suggested changes are non-breaking, I'm happy to make them. Otherwise I suggest that I do the changes in a separate CL, so that if things break we can roll back without breaking downstream build. Please let me know whether you prefer 2 CLs or one, and I'll upload/land as appropriate. Cheers, Vaclav
On 2016/07/13 17:17:43, vabr (Chromium) wrote: > On 2016/07/13 15:22:27, Eugene But wrote: > > lgtm > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > File ios/web/public/test/web_js_test.h (right): > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > ios/web/public/test/web_js_test.h:23: : java_script_paths_([java_script_paths > > retain]) {} > > NIT: Consider copying array since the input can be mutable. > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > ios/web/public/test/web_js_test.h:63: // Wait until main web injection has > > occured. > > Do we still need this "wait"? I would assume that web JS is injected > immediately > > into WKWebView. > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > ios/web/public/test/web_js_test.h:72: > > WebTestT::EvaluateJavaScriptAsString([NSString > > NIT: EvaluateJavaScriptAsString is marked as deprecated. Can you replace this > > with |ExecuteJavaScript|? > > > > We plan to clean this up later, so if you prefer to leave the code as it is > it's > > fine. > > Thanks, Eugene! > > To be honest, I have not much experience with this particular template. If you > are sure the suggested changes are non-breaking, I'm happy to make them. > Otherwise I suggest that I do the changes in a separate CL, so that if things > break we can roll back without breaking downstream build. > > Please let me know whether you prefer 2 CLs or one, and I'll upload/land as > appropriate. > > Cheers, > Vaclav I'm not confident that my suggestions will not break anything so a separate CL for "JS injection waiting" sounds like the best option. As for EvaluateJavaScriptAsString vs. ExecuteJavaScript you can just leave it as is.
On 2016/07/13 17:22:44, Eugene But wrote: > On 2016/07/13 17:17:43, vabr (Chromium) wrote: > > On 2016/07/13 15:22:27, Eugene But wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > > File ios/web/public/test/web_js_test.h (right): > > > > > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > > ios/web/public/test/web_js_test.h:23: : > java_script_paths_([java_script_paths > > > retain]) {} > > > NIT: Consider copying array since the input can be mutable. > > > > > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > > ios/web/public/test/web_js_test.h:63: // Wait until main web injection has > > > occured. > > > Do we still need this "wait"? I would assume that web JS is injected > > immediately > > > into WKWebView. > > > > > > > > > https://codereview.chromium.org/2150513002/diff/60001/ios/web/public/test/web... > > > ios/web/public/test/web_js_test.h:72: > > > WebTestT::EvaluateJavaScriptAsString([NSString > > > NIT: EvaluateJavaScriptAsString is marked as deprecated. Can you replace > this > > > with |ExecuteJavaScript|? > > > > > > We plan to clean this up later, so if you prefer to leave the code as it is > > it's > > > fine. > > > > Thanks, Eugene! > > > > To be honest, I have not much experience with this particular template. If you > > are sure the suggested changes are non-breaking, I'm happy to make them. > > Otherwise I suggest that I do the changes in a separate CL, so that if things > > break we can roll back without breaking downstream build. > > > > Please let me know whether you prefer 2 CLs or one, and I'll upload/land as > > appropriate. > > > > Cheers, > > Vaclav > I'm not confident that my suggestions will not break anything so a separate CL > for "JS injection waiting" sounds like the best option. As for > EvaluateJavaScriptAsString vs. ExecuteJavaScript you can just leave it as is. Thanks! In that case I'll land this (to make sure its rolled soon and I can land the downstream patch) and in the meantime will put together the other CL to address your comments. I'm happy to leave EvaluateJavaScriptAsString for the planned clean-up if you say so. :) Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2150513002/#ps60001 (title: "include -> import")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move web_js_test.h upstream This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into the ios_web_test_support build target (+equivalent in GN). The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. BUG=627775 ========== to ========== Move web_js_test.h upstream This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into the ios_web_test_support build target (+equivalent in GN). The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. BUG=627775 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Move web_js_test.h upstream This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into the ios_web_test_support build target (+equivalent in GN). The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. BUG=627775 ========== to ========== Move web_js_test.h upstream This creates a copy of the downstream web_js_test.h upstream in ios/web/public/test and puts it into the ios_web_test_support build target (+equivalent in GN). The upstream web_js_test.h is an almost-identical copy of the downstream version, with 2 differences: (1) git cl format is required upstream, which changed indenting in a single location. (2) namespace web was introduced to match the style of the target directory. BUG=627775 Committed: https://crrev.com/b737caa2d98e0a27d283cd66ab777f9b42e042a2 Cr-Commit-Position: refs/heads/master@{#405217} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b737caa2d98e0a27d283cd66ab777f9b42e042a2 Cr-Commit-Position: refs/heads/master@{#405217}
Message was sent while issue was closed.
Follow-up CL is https://codereview.chromium.org/2148723003. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
