Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556023002:
Added flags for PrintBrowser mode (Closed)
DescriptionAdded flags for PrintBrowser mode
Added the build flag enable_print_browser (defaults to same as
enable_print_preview).
Added the chrome feature flag kPrintBrowser.
Serves as foundation for crrev.com/2566153007
BUG=667547
Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82nVmoWbucz1LYZtu8/edit?usp=sharing
Patch Set 1 #Patch Set 2 : Replaced enable-print-browser feature flag with command line switch. #
Total comments: 4
Patch Set 3 : Replaced enable-print-browser feature flag with command line switch. #Patch Set 4 : Replaced enable-print-browser feature flag with command line switch. #
Total comments: 1
Patch Set 5 : Added histograms.xml entry for enable-print-browser #Patch Set 6 : Fix git weirdness #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 36 (17 generated)
Description was changed from ========== Added flags for PrintBrowser mode Added the build flag enable_print_browser (defaults to same as enable_print_preview). Added the chrome feature flag kPrintBrowser. BUG=667547 ========== to ========== Added flags for PrintBrowser mode Added the build flag enable_print_browser (defaults to same as enable_print_preview). Added the chrome feature flag kPrintBrowser. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... ==========
gozzard@google.com changed reviewers: + vitalybuka@chromium.org
PTAL
The CQ bit was checked by vitalybuka@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...
On 2016/12/07 05:34:47, gozzard wrote: > PTAL This patch just declares unused build flag. It would be good to see what is exactly going to be protected by this flag, maybe as separate CL. I am not sure who reviewed the design doc. I would be nice to check with the current printing owner thestig@, but he is currently OOO. "Intent to implement" to chromium-dev also could be helpful.
https://codereview.chromium.org/2556023002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2556023002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:5173: Inconsistency: scaling vs browser feature https://codereview.chromium.org/2556023002/diff/20001/printing/features/featu... File printing/features/features.gni (right): https://codereview.chromium.org/2556023002/diff/20001/printing/features/featu... printing/features/features.gni:20: # Enable printing mode browsing. Implies enable_print_preview. Code implies opposite.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Added flags for PrintBrowser mode Added the build flag enable_print_browser (defaults to same as enable_print_preview). Added the chrome feature flag kPrintBrowser. BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... ========== to ========== Added flags for PrintBrowser mode Added the build flag enable_print_browser (defaults to same as enable_print_preview). Added the chrome feature flag kPrintBrowser. Serves as foundation for crrev.com/2556023002 BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... ==========
Thanks for the feedback. This issue serves as foundation for crrev.com/2556023002 Most of the logic for PrintBrowser is there PTAL https://codereview.chromium.org/2556023002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2556023002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:5173: On 2016/12/12 at 05:29:48, Vitaly Buka wrote: > Inconsistency: scaling vs browser feature Fixed https://codereview.chromium.org/2556023002/diff/20001/printing/features/featu... File printing/features/features.gni (right): https://codereview.chromium.org/2556023002/diff/20001/printing/features/featu... printing/features/features.gni:20: # Enable printing mode browsing. Implies enable_print_preview. On 2016/12/12 at 05:29:48, Vitaly Buka wrote: > Code implies opposite. The implication logic is in another file. This determines the default value for the build flag. Fixed
> This issue serves as foundation for crrev.com/2556023002 This is the same review.
Description was changed from ========== Added flags for PrintBrowser mode Added the build flag enable_print_browser (defaults to same as enable_print_preview). Added the chrome feature flag kPrintBrowser. Serves as foundation for crrev.com/2556023002 BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... ========== to ========== Added flags for PrintBrowser mode Added the build flag enable_print_browser (defaults to same as enable_print_preview). Added the chrome feature flag kPrintBrowser. Serves as foundation for crrev.com/2566153007 BUG=667547 Design Doc: https://docs.google.com/a/google.com/document/d/1pRu5Lh4SGelhbsqfrLnLQov0o82n... ==========
On 2016/12/14 at 23:25:06, vitalybuka wrote: > > This issue serves as foundation for crrev.com/2556023002 > > This is the same review. Fixed
Please move design doc link into the bug
https://codereview.chromium.org/2556023002/diff/60001/printing/features/featu... File printing/features/features.gni (right): https://codereview.chromium.org/2556023002/diff/60001/printing/features/featu... printing/features/features.gni:18: What is the purpose of ENABLE_PRINT_BROWSER if its value is the same as ENABLE_PRINT_PREVIEW? Maybe just use ENABLE_PRINT_PREVIEW?
On 2016/12/15 at 00:19:49, vitalybuka wrote: > https://codereview.chromium.org/2556023002/diff/60001/printing/features/featu... > File printing/features/features.gni (right): > > https://codereview.chromium.org/2556023002/diff/60001/printing/features/featu... > printing/features/features.gni:18: > What is the purpose of ENABLE_PRINT_BROWSER if its value is the same as ENABLE_PRINT_PREVIEW? > > Maybe just use ENABLE_PRINT_PREVIEW? Only the default value of ENABLE_PRINT_BROWSER is tied to ENABLE_PRINT_PREVIEW This method makes it possible to build without PrintBrowser mode by explicitly setting enable_print_browser=false Similarly, it is possible to, with a trivial change, make it so PrintBrowser does not build by default
> Only the default value of ENABLE_PRINT_BROWSER is tied to ENABLE_PRINT_PREVIEW > This method makes it possible to build without PrintBrowser mode by explicitly > setting enable_print_browser=false > Similarly, it is possible to, with a trivial change, make it so PrintBrowser > does not build by default Than who is going to build this config (ENABLE_PRINT_BROWSER=1)?
On 2016/12/15 02:58:36, Vitaly Buka wrote: > > Only the default value of ENABLE_PRINT_BROWSER is tied to ENABLE_PRINT_PREVIEW > > This method makes it possible to build without PrintBrowser mode by explicitly > > setting enable_print_browser=false > > Similarly, it is possible to, with a trivial change, make it so PrintBrowser > > does not build by default > > Than who is going to build this config (ENABLE_PRINT_BROWSER=1)? Sorry, who is going to build ENABLE_PRINT_BROWSER=0 if 1 is default?
On 2016/12/15 at 02:59:38, vitalybuka wrote: > On 2016/12/15 02:58:36, Vitaly Buka wrote: > > > Only the default value of ENABLE_PRINT_BROWSER is tied to ENABLE_PRINT_PREVIEW > > > This method makes it possible to build without PrintBrowser mode by explicitly > > > setting enable_print_browser=false > > > Similarly, it is possible to, with a trivial change, make it so PrintBrowser > > > does not build by default > > > > Than who is going to build this config (ENABLE_PRINT_BROWSER=1)? > > Sorry, who is going to build ENABLE_PRINT_BROWSER=0 if 1 is default? The default was selected somewhat arbitrarily, but I think it is important to have the option to not build the feature. If you think it would be better to not build it by default, I can and will happily change the default.
The CQ bit was checked by gozzard@google.com 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
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by gozzard@google.com 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
Dry run: This issue passed the CQ dry run.
> The default was selected somewhat arbitrarily, but I think it is important to > have the option to not build the feature. We should have only options which are being used. Unused options only make code maintenance harder. > If you think it would be better to not build it by default, I can and will > happily change the default. If it's not enabled by default we need a bot which will build and test this config or this is a code which will quickly degrade. Unfortunately I don't have a big picture of how PrintBrowser is going to be used. Maybe you could extend your design doc and cover how this is going to help Chromium.
On 2016/12/15 at 08:08:10, vitalybuka wrote: > > The default was selected somewhat arbitrarily, but I think it is important to > > have the option to not build the feature. > > We should have only options which are being used. Unused options only make code maintenance harder. > > > If you think it would be better to not build it by default, I can and will > > happily change the default. > > If it's not enabled by default we need a bot which will build and test this config or this is a code which will quickly degrade. > > Unfortunately I don't have a big picture of how PrintBrowser is going to be used. Maybe you could extend your design > doc and cover how this is going to help Chromium. Are you aware of any Chromium standard for where to use build flags and where not to? My logic was that this is a feature that may only be used for the test infrastructure mentioned in the design doc, and hence we probably don't want it building into release builds. Having said that I have no idea what the normal way of handling such features is in Chromium, hence my desire to know if there is a written standard anywhere. I will look into expanding the design doc as you asked as my project moves into its second phase, but at this time I am unfortunately unable to offer you any more detail on the big picture for PrintBrowser.
vitalybuka@chromium.org changed reviewers: + rbpotter@chromium.org, skau@chromium.org
+rbpotter +skau Same here
I would recommend defaulting the flag to false and validating it on all the platforms and sending out an ITI on chromium-dev to determine the best bot to use this feature. https://codereview.chromium.org/2556023002/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2556023002/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:404: // Enables the Print Browser feature in print preview. Is this accurate? It seems like this feature affects more than print preview. https://codereview.chromium.org/2556023002/diff/100001/printing/features/BUIL... File printing/features/BUILD.gn (right): https://codereview.chromium.org/2556023002/diff/100001/printing/features/BUIL... printing/features/BUILD.gn:17: enable_print_preview = true instead of forcing print_preview to be true, can you require that it's true and throw an error otherwise. This behavior seems surprising. https://codereview.chromium.org/2556023002/diff/100001/printing/features/feat... File printing/features/features.gni (right): https://codereview.chromium.org/2556023002/diff/100001/printing/features/feat... printing/features/features.gni:21: enable_print_browser = enable_print_preview It is surprising that print preview implies print browser is enabled. Please default the value to false. Print Preview is enabled on most platforms so the feature will effectively be enabled for most platforms.
I would recommend defaulting the flag to false and validating it on all the platforms and sending out an ITI on chromium-dev to determine the best bot to use this feature. https://codereview.chromium.org/2556023002/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2556023002/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:404: // Enables the Print Browser feature in print preview. Is this accurate? It seems like this feature affects more than print preview. https://codereview.chromium.org/2556023002/diff/100001/printing/features/BUIL... File printing/features/BUILD.gn (right): https://codereview.chromium.org/2556023002/diff/100001/printing/features/BUIL... printing/features/BUILD.gn:17: enable_print_preview = true instead of forcing print_preview to be true, can you require that it's true and throw an error otherwise. This behavior seems surprising. https://codereview.chromium.org/2556023002/diff/100001/printing/features/feat... File printing/features/features.gni (right): https://codereview.chromium.org/2556023002/diff/100001/printing/features/feat... printing/features/features.gni:21: enable_print_browser = enable_print_preview It is surprising that print preview implies print browser is enabled. Please default the value to false. Print Preview is enabled on most platforms so the feature will effectively be enabled for most platforms.
On 2016/12/16 at 18:05:53, skau wrote: > I would recommend defaulting the flag to false and validating it on all the platforms and sending out an ITI on chromium-dev to determine the best bot to use this feature. Can do. Consider it on my list. > https://codereview.chromium.org/2556023002/diff/100001/chrome/common/chrome_s... > File chrome/common/chrome_switches.cc (right): > > https://codereview.chromium.org/2556023002/diff/100001/chrome/common/chrome_s... > chrome/common/chrome_switches.cc:404: // Enables the Print Browser feature in print preview. > Is this accurate? It seems like this feature affects more than print preview. Happy to change it if you have something you would prefer, but ultimately Print Browser just triggers print preview, so I don't see the problem. > https://codereview.chromium.org/2556023002/diff/100001/printing/features/BUIL... > File printing/features/BUILD.gn (right): > > https://codereview.chromium.org/2556023002/diff/100001/printing/features/BUIL... > printing/features/BUILD.gn:17: enable_print_preview = true > instead of forcing print_preview to be true, can you require that it's true and throw an error otherwise. This behavior seems surprising. Excellent idea. I don't know if gn builds support this but will look into it. > https://codereview.chromium.org/2556023002/diff/100001/printing/features/feat... > File printing/features/features.gni (right): > > https://codereview.chromium.org/2556023002/diff/100001/printing/features/feat... > printing/features/features.gni:21: enable_print_browser = enable_print_preview > It is surprising that print preview implies print browser is enabled. Please default the value to false. Print Preview is enabled on most platforms so the feature will effectively be enabled for most platforms. As above, can do. As you say we will need to find a build bot for it then. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
