Extensions Docs Server: HTML parser in IDS

Extensions Docs Server: HTML parser in IDS

Replaced the regex in IntroDataSource with an HTML parser.

BUG=131095
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148014

[cduvall, 2012-07-20 23:26:39]

s/regex/html parser

[not at google - send to devlin, 2012-07-23 12:47:36]

http://codereview.chromium.org/10810047/diff/3001/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

http://codereview.chromium.org/10810047/diff/3001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/intro_data_source.py:18: """
I think the pattern is like

def __init_(self):
  HTMLParser.__init__(self)
  self.toc = []
  ...

http://codereview.chromium.org/10810047/diff/3001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/intro_data_source.py:20: self.page_title =
''
None not empty string? We may want to test for the existence of a title, and
this would make it a bit ambiguous.

http://codereview.chromium.org/10810047/diff/3001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/intro_data_source.py:22: self._current =
{}
current_what?

http://codereview.chromium.org/10810047/diff/3001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/intro_data_source.py:39: return
why is this needed?

http://codereview.chromium.org/10810047/diff/3001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/intro_data_source.py:43:
self._current['title'] = data
Note that this won't handle cases like

<h2>This heading is <b>extra important</b></h2>

You might need to maintain a "header_tag_stack" and rather than accessing
recent_tag, access the top of that.

[cduvall, 2012-07-23 20:24:21]

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:18: """
On 2012/07/23 12:47:36, kalman wrote:
> I think the pattern is like
> 
> def __init_(self):
>   HTMLParser.__init__(self)
>   self.toc = []
>   ...

Done.

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:20: self.page_title =
''
On 2012/07/23 12:47:36, kalman wrote:
> None not empty string? We may want to test for the existence of a title, and
> this would make it a bit ambiguous.

I looked all throughout the internet for this, and I only found the super(Foo,
self).__init__() version. Thanks.

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:22: self._current =
{}
On 2012/07/23 12:47:36, kalman wrote:
> current_what?

Done.

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:39: return
On 2012/07/23 12:47:36, kalman wrote:
> why is this needed?

Not needed anymore.

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:43:
self._current['title'] = data
On 2012/07/23 12:47:36, kalman wrote:
> Note that this won't handle cases like
> 
> <h2>This heading is <b>extra important</b></h2>
> 
> You might need to maintain a "header_tag_stack" and rather than accessing
> recent_tag, access the top of that.

I ended up not using a stack, but the new version handles tags in the headings.
It doesn't handle headings inside of headings, but that should never happen. I
can change it to use a stack instead if you would like.

[not at google - send to devlin, 2012-07-23 23:30:41]

lgtm

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:43:
self._current['title'] = data
On 2012/07/23 20:24:21, cduvall wrote:
> On 2012/07/23 12:47:36, kalman wrote:
> > Note that this won't handle cases like
> > 
> > <h2>This heading is <b>extra important</b></h2>
> > 
> > You might need to maintain a "header_tag_stack" and rather than accessing
> > recent_tag, access the top of that.
> 
> I ended up not using a stack, but the new version handles tags in the
headings.
> It doesn't handle headings inside of headings, but that should never happen. I
> can change it to use a stack instead if you would like.

sgtm. I realise I started micro-managing you a bit recently, will try to stop :)

https://chromiumcodereview.appspot.com/10810047/diff/3002/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10810047/diff/3002/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:42:
self.handle_data('<' + tag + '/>')
I think we should just strip out the tags, like, if the title was

<h1>this is <b>very</b> important</h1>

then we'd want the title to be <title>this is very important</title> -- putting
HTML in there would look quite strange.

OTOH having html in the TOC would be nice, just, simpler if we forget about it.

https://chromiumcodereview.appspot.com/10810047/diff/3002/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:51: self.page_title
+= data
so if there are multiple <h1> tags it concats them together? I think that would
end up looking strange; just use the first one.

[cduvall, 2012-07-23 23:58:33]

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10810047/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:43:
self._current['title'] = data
On 2012/07/23 23:30:41, kalman wrote:
> On 2012/07/23 20:24:21, cduvall wrote:
> > On 2012/07/23 12:47:36, kalman wrote:
> > > Note that this won't handle cases like
> > > 
> > > <h2>This heading is <b>extra important</b></h2>
> > > 
> > > You might need to maintain a "header_tag_stack" and rather than accessing
> > > recent_tag, access the top of that.
> > 
> > I ended up not using a stack, but the new version handles tags in the
> headings.
> > It doesn't handle headings inside of headings, but that should never happen.
I
> > can change it to use a stack instead if you would like.
> 
> sgtm. I realise I started micro-managing you a bit recently, will try to stop
:)

Np, I would much rather learn to do it the right way :)

https://chromiumcodereview.appspot.com/10810047/diff/3002/chrome/common/exten...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10810047/diff/3002/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:42:
self.handle_data('<' + tag + '/>')
On 2012/07/23 23:30:42, kalman wrote:
> I think we should just strip out the tags, like, if the title was
> 
> <h1>this is <b>very</b> important</h1>
> 
> then we'd want the title to be <title>this is very important</title> --
putting
> HTML in there would look quite strange.
> 
> OTOH having html in the TOC would be nice, just, simpler if we forget about
it.

Done.

https://chromiumcodereview.appspot.com/10810047/diff/3002/chrome/common/exten...
chrome/common/extensions/docs/server2/intro_data_source.py:51: self.page_title
+= data
On 2012/07/23 23:30:42, kalman wrote:
> so if there are multiple <h1> tags it concats them together? I think that
would
> end up looking strange; just use the first one.

Done.