inject_misc_tags no longer fails if document has no head. Resolves #16.

inject_misc_tags no longer fails if document has no head. Resolves #16.

https://github.com/chromium/caterpillar/issues/16

R=mgiuca@chromium.org

Committed: 5884063f4dc2a0943ab8f8f227a7c799f0dc0e36

[Matthew Alger, 2016-01-28 04:26:55]

alger@google.com changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2016-01-28 06:56:25]

https://codereview.chromium.org/1644543005/diff/1/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644543005/diff/1/src/caterpillar.py#newcode283
src/caterpillar.py:283: target_tag = soup.head if soup.head else soup
Maybe this should create a head tag, instead of just injecting them straight
into the document?

[Matthew Alger, 2016-01-28 23:08:44]

I've added some tests for this bug, since the tests CL has now landed.

https://codereview.chromium.org/1644543005/diff/1/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644543005/diff/1/src/caterpillar.py#newcode283
src/caterpillar.py:283: target_tag = soup.head if soup.head else soup
On 2016/01/28 06:56:25, Matt Giuca wrote:
> Maybe this should create a head tag, instead of just injecting them straight
> into the document?

Good idea - done.

[Matt Giuca, 2016-01-29 02:19:26]

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar.py#newc...
src/caterpillar.py:289: if not soup.head:
head = soup.head
if not head:
  ...

Now you are guaranteed to have a "head" variable and you can replace instances
of "soup.head" below with just "head".

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar_test.py
File src/caterpillar_test.py (right):

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar_test.py...
src/caterpillar_test.py:454: html = """
"""\

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar_test.py...
src/caterpillar_test.py:465: link = soup.find('link', rel='manifest')
Again, I think you should match the entire file, not just check that the link
was found somewhere in the document.

[Matthew Alger, 2016-01-29 02:41:51]

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar.py#newc...
src/caterpillar.py:289: if not soup.head:
On 2016/01/29 02:19:26, Matt Giuca wrote:
> head = soup.head
> if not head:
>   ...
> 
> Now you are guaranteed to have a "head" variable and you can replace instances
> of "soup.head" below with just "head".

Okay, done. I'm not sure I see the benefit in this.

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar_test.py
File src/caterpillar_test.py (right):

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar_test.py...
src/caterpillar_test.py:454: html = """
On 2016/01/29 02:19:26, Matt Giuca wrote:
> """\

Done.

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar_test.py...
src/caterpillar_test.py:465: link = soup.find('link', rel='manifest')
On 2016/01/29 02:19:26, Matt Giuca wrote:
> Again, I think you should match the entire file, not just check that the link
> was found somewhere in the document.

Done.

[Matt Giuca, 2016-01-29 03:04:13]

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar.py
File src/caterpillar.py (right):

https://codereview.chromium.org/1644543005/diff/40001/src/caterpillar.py#newc...
src/caterpillar.py:289: if not soup.head:
On 2016/01/29 02:41:51, Matthew Alger wrote:
> On 2016/01/29 02:19:26, Matt Giuca wrote:
> > head = soup.head
> > if not head:
> >   ...
> > 
> > Now you are guaranteed to have a "head" variable and you can replace
instances
> > of "soup.head" below with just "head".
> 
> Okay, done. I'm not sure I see the benefit in this.

Mostly performance.

You generally want to avoid doing the same operation multiple times (and instead
storing it in a variable). Whether that's computations or just lookups, the same
principle applies (but it's less of a problem for lookups).

In Python (unlike static languages), member lookups are hash table lookups so
they are not free. And I don't know for sure, but I'd guess that this particular
lookup (in BeautifulSoup) may actually be an HTML tree search rather than a
simple hash lookup.

(Note: You should generally avoid micro-optimizations unless you have a pressing
performance need. But this sort of thing is more of a matter of good style/habit
than us specifically needing performance in this part of the program.)

[Matt Giuca, 2016-01-29 03:04:18]

lgtm

[Matthew Alger, 2016-01-29 03:18:54]

Description was changed from

==========
inject_misc_tags no longer fails if document has no head. Resolves #16.

https://github.com/chromium/caterpillar/issues/16
==========

to

==========
inject_misc_tags no longer fails if document has no head. Resolves #16.

https://github.com/chromium/caterpillar/issues/16

R=mgiuca@chromium.org

Committed: 5884063f4dc2a0943ab8f8f227a7c799f0dc0e36
==========

[Matthew Alger, 2016-01-29 03:18:57]

Committed patchset #4 (id:60001) manually as
5884063f4dc2a0943ab8f8f227a7c799f0dc0e36 (presubmit successful).