<div dir="ltr">Hi Kristof,<br><br>Generally LGTM, but this line looks a bit dodgy:<div><br></div><div><div>+{% if selector_part_to_search is none %}</div></div><div><br></div><div>shouldn't None have a capital? Have you checked this works?</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, 9 Jun 2015 at 18:03 Kristof Beyls <<a href="mailto:kristof.beyls@arm.com">kristof.beyls@arm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Ah, I get your point now – indeed that’s much better.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I’ve attached new versions of the patches will all your comments addressed.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks!<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Kristof<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> James Molloy [mailto:<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>] <br><b>Sent:</b> 08 June 2015 15:47</span></p></div></div></div></div></div><div lang="EN-GB" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""><br><b>To:</b> Kristof Beyls; Chris Matthews<br><b>Cc:</b> llvm-commits<br><b>Subject:</b> Re: [PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.<u></u><u></u></span></p></div></div></div></div></div><div lang="EN-GB" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div><div><blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"><p class="MsoNormal" style="margin-bottom:12.0pt">> 0003 and 0004: Instead of trying to work out the column to search on, I think it would be easier if you would just give all cells that are searchable some extra class, such as "td-searchable". Then, you wouldn't need to parameterize the build_regex macro; it could just always do something like "$(this).find('td.td-searchable').text()". How does that sound? Especially in 0004, this might simplify things.<br><br>If done that way, a mechanism will need to be found to figure out from the selected "td" that matches, which is the DOM element that needs<br>to be hidden (so far those are all <tr>s containing the <td>). One way or another, there needs to be a link between the DOM element that needs<br>to be hidden/shown, and which part of it that needs to be searched for the regex. As long as this functionality will only hide/show table<br>rows and search table cells, the alternative you propose may end up being a bit simpler. The way it's implemented now, it's probably a bit<br>more generic. I guess that the best solution depends on whether in the future we might want to do regex searches to hide/show stuff<br>other than table rows and cells.<u></u><u></u></p></blockquote></div></div></div></div></div></div><div lang="EN-GB" link="blue" vlink="purple"><div><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"><div><div><div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">Yes I agree, both mechanisms need parameterising somehow. But my suggested method would make it possible to implement all our current use-cases with just default parameters. I really don't like the way that in your current solution we have to store the table layout in two locations - (1) in the HTML <table> markup and (2) in the parameters to the macro. Having the JS grab the information it needs from the DOM (as long as it's not too costly) seems to me to be a better design.<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">James<u></u><u></u></p></div></div></div></div></div></div></div></blockquote></div>