Modified grit to support <if expr="..."> tags within included html files

Modified grit to support <if expr="..."> tags within included html files. Removed ChromeOS-specific sub-pages from options by using the newly added if tag. Changed <!--include file="..."--> tag to <include src="...">.

BUG=chromium-os:4450
TEST=make sure that chromeos specific pages don't show in chrome:options for other platforms


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51578

[Jói, 2010-07-02 10:10:55]

I'd like to see unit tests added for the GRIT changes.  The approach
looks fine to me.

Cheers,
Jói


On Fri, Jul 2, 2010 at 8:04 AM,  <zelidrag@chromium.org> wrote:
> Reviewers: Chris Silverberg, dhg, Jói,
>
> Description:
> Modified grit to support <if expr="..."> tags within included html files.
> Removed ChromeOS-specific sub-pages from options by using the newly added if
> tag.
>
> BUG=chromium-os:4450
> TEST=make sure that chromeos specific pages don't show in chrome:options for
> other platforms
>
>
> Please review this at http://codereview.chromium.org/2872028/show
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/resources/options.html
>  M     tools/grit/grit/format/html_inline.py
>  M     tools/grit/grit/format/rc.py
>
>
> Index: chrome/browser/resources/options.html
> ===================================================================
> --- chrome/browser/resources/options.html       (revision 51203)
> +++ chrome/browser/resources/options.html       (working copy)
> @@ -16,7 +16,9 @@
>  <script src="options/preferences.js"></script>
>  <script src="options/pref_ui.js"></script>
>  <script src="options/options_page.js"></script>
> -<script src="options/chromeos_system_options.js"></script>
> +<if expr="pp_ifdef('chromeos')">
> +  <script src="options/chromeos_system_options.js"></script>
> +</if>
>  <script>
>
>
 ///////////////////////////////////////////////////////////////////////////////
> @@ -142,7 +144,9 @@
>      </ul>
>   </div>
>   <div id="mainview">
> -     <!-- include file="options/chromeos_system_options.html" -->
> +     <if expr="pp_ifdef('chromeos')">
> +        <!-- include file="options/chromeos_system_options.html" -->
> +     </if>
>      <div class="page hidden" id="dummyPage">
>         <!-- TODO(zelidrag): remove this one once we get another page here
> -->
>         Dummy page
> Index: tools/grit/grit/format/html_inline.py
> ===================================================================
> --- tools/grit/grit/format/html_inline.py       (revision 51203)
> +++ tools/grit/grit/format/html_inline.py       (working copy)
> @@ -19,6 +19,8 @@
>  import base64
>  import mimetypes
>
> +from grit.node import base
> +
>  DIST_DEFAULT = 'chromium'
>  DIST_ENV_VAR = 'CHROMIUM_BUILD'
>  DIST_SUBSTR = '%DISTRIBUTION%'
> @@ -67,7 +69,7 @@
>   prefix = src_match.string[src_match.start():src_match.start('filename')-1]
>   return "%s\"data:%s;base64,%s\"" % (prefix, mimetype, inline_data)
>
> -def InlineFile(input_filename, output_filename):
> +def InlineFile(input_filename, output_filename, grd_node):
>   """Inlines the resources in a specified file.
>
>   Reads input_filename, finds all the src attributes and attempts to
> @@ -100,6 +102,16 @@
>     filename = filename.replace('%DISTRIBUTION%', distribution)
>     return os.path.join(input_filepath, filename)
>
> +  def IsConditionSatisfied(src_match):
> +    expression = src_match.group('expression')
> +    return grd_node is None or grd_node.EvaluateCondition(expression)
> +
> +  def CheckConditionalElements(src_match):
> +    """Helper function to conditionally inline inner elements"""
> +    if not IsConditionSatisfied(src_match):
> +      return ''
> +    return src_match.group('content')
> +
>   def InlineFileContents(src_match, pattern):
>     """Helper function to inline external script and css files"""
>     filepath = GetFilepath(src_match)
> @@ -144,7 +156,6 @@
>                   lambda m: SrcReplace(m, filepath),
>                   text)
>
> -
>   # We need to inline css and js before we inline images so that image
>   # references gets inlined in the css and js
>   flat_text = re.sub('<script .*?src="(?P<filename>[^"\']*)".*?></script>',
> @@ -161,6 +172,12 @@
>       InlineIncludeFiles,
>       flat_text)
>
> +  # Check conditional elements, remove unsatisfied ones from the file.
> +  flat_text = re.sub('<if .*?expr="(?P<expression>[^"]*)".*?>\s*' +
> +                     '(?P<content>([\s\S]+?))\s*</if>',
> +                     CheckConditionalElements,
> +                     flat_text)
> +
>   # TODO(glen): Make this regex not match src="" text that is not inside a
> tag
>   flat_text = re.sub('src="(?P<filename>[^"\']*)"',
>                      SrcReplace,
> Index: tools/grit/grit/format/rc.py
> ===================================================================
> --- tools/grit/grit/format/rc.py        (revision 51178)
> +++ tools/grit/grit/format/rc.py        (working copy)
> @@ -427,7 +427,7 @@
>     if self.flatten_html:
>       # Generate the flattened HTML file.
>       flat_filename = os.path.join(output_dir, os.path.basename(filename))
> -      html_inline.InlineFile(filename, flat_filename)
> +      html_inline.InlineFile(filename, flat_filename, item)
>
>       # Include the flattened HTML file.
>       filename = os.path.basename(filename)
>
>
>

[arv (Not doing code reviews), 2010-07-02 16:32:38]

I think it is a bit inconsistent that the include is done with a comment but the
conditional is done as a tag. Can we change the include to?

<include src="path_to_src"/>

[csilv, 2010-07-02 18:01:31]

http://codereview.chromium.org/2872028/diff/2001/3002
File tools/grit/grit/format/html_inline.py (right):

http://codereview.chromium.org/2872028/diff/2001/3002#newcode82
tools/grit/grit/format/html_inline.py:82: """
suggest adding grd_node to the args list descriptions.

[zel, 2010-07-02 20:34:36]

I have also added until test per Joi's comments and renamed include tag as arv
suggested.

http://codereview.chromium.org/2872028/diff/2001/3002
File tools/grit/grit/format/html_inline.py (right):

http://codereview.chromium.org/2872028/diff/2001/3002#newcode82
tools/grit/grit/format/html_inline.py:82: """
On 2010/07/02 18:01:31, Chris Silverberg wrote:
> suggest adding grd_node to the args list descriptions.

Done.

[dhg, 2010-07-02 23:01:03]

LGTM
On 2010/07/02 20:34:36, zel wrote:
> I have also added until test per Joi's comments and renamed include tag as arv
> suggested.
> 
> http://codereview.chromium.org/2872028/diff/2001/3002
> File tools/grit/grit/format/html_inline.py (right):
> 
> http://codereview.chromium.org/2872028/diff/2001/3002#newcode82
> tools/grit/grit/format/html_inline.py:82: """
> On 2010/07/02 18:01:31, Chris Silverberg wrote:
> > suggest adding grd_node to the args list descriptions.
> 
> Done.

[arv (Not doing code reviews), 2010-07-02 23:03:21]

lgtm

[Jói, 2010-07-03 00:14:44]

lgtm

On Jul 2, 2010 11:03 PM, <arv@chromium.org> wrote:
> lgtm
>
> http://codereview.chromium.org/2872028/show

[csilv, 2010-07-03 00:17:13]

lgtm

[csilv, 2010-07-03 00:18:49]

regarding the syntax change for the include files, does that impact other files
already in use or grit pretty new to the project?

[arv (Not doing code reviews), 2010-07-03 00:26:45]

The include syntax was added only a few days ago and there should be
no other uses yet.

erik



On Fri, Jul 2, 2010 at 17:18,  <csilv@chromium.org> wrote:
> regarding the syntax change for the include files, does that impact other
> files
> already in use or grit pretty new to the project?
>
> http://codereview.chromium.org/2872028/show
>

[zel, 2010-07-03 00:48:23]

That is correct, my CL is the first one that introduced the new include and
if tags.


On Fri, Jul 2, 2010 at 5:26 PM, Erik Arvidsson <arv@chromium.org> wrote:

> The include syntax was added only a few days ago and there should be
> no other uses yet.
>
> erik
>
>
>
> On Fri, Jul 2, 2010 at 17:18,  <csilv@chromium.org> wrote:
> > regarding the syntax change for the include files, does that impact other
> > files
> > already in use or grit pretty new to the project?
> >
> > http://codereview.chromium.org/2872028/show
> >
>