[gwt-contrib] Re: Reduces size of the compile report (soyc) for large projects with many (issue1171801)

LGTM, but please not my comments... SOYC is becoming smart!! One thing I'm wondering is whether we have a good way of making sure we still get the right information everywhere (the smarter it gets, the harder it gets to follow in some sense, e.g., one-character function names, etc.). There's

[gwt-contrib] Re: Reduces the size of the soyc report by combining many s (issue1139801)

LGTM. This is another great change - thanks! http://gwt-code-reviews.appspot.com/1139801/diff/1/2 File dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java (right): http://gwt-code-reviews.appspot.com/1139801/diff/1/2#newcode789

[gwt-contrib] Re: Makes part of the Compiler Report (SOYC) smaller by (issue1123801)

LGTM. Nice! I would probably just add a test for empty strings. http://gwt-code-reviews.appspot.com/1123801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Makes part of the Compiler Report (SOYC) smaller by (issue1123801)

http://gwt-code-reviews.appspot.com/1123801/diff/1/2 File dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java (right): http://gwt-code-reviews.appspot.com/1123801/diff/1/2#newcode879 dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:879: return

[gwt-contrib] Re: Makes part of the Compiler Report (SOYC) smaller by (issue1123801)

Really sorry to be so picky, but if you call getClassSubstring(String fullMethodName) with something like myClass, it'll throw an IndexOutOfBoundsException (start longer than end). It'll also fail for the empty string (end: 0, start: 1). BTW, I can't see the side-by-side diff, not sure if

[gwt-contrib] Re: Remove extra slash from request URL of Showcase source files to be compatible with servers that ... (issue961801)

LGTM http://gwt-code-reviews.appspot.com/961801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Making Showcase generate a hidden site map to Showcase to make it crawlable. Also ensuring that ... (issue952801)

LGTM http://gwt-code-reviews.appspot.com/952801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Converts two new files from dos EOL format to unix EOL format. (issue854801)

Reviewers: kjin, Description: Converts two new files from dos EOL format to unix EOL format. Review by: k...@google.com Please review this at http://gwt-code-reviews.appspot.com/854801/show Affected files: M user/src/com/google/gwt/validation/rebind/GwtSpecificValidatorGenerator.java M

[gwt-contrib] Re: Delay start of history token transitions in HistoryTest.testHistory() so that when tokens are lo... (issue850802)

LGTM http://gwt-code-reviews.appspot.com/850802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixes a bug in the compile report dashboard where entries with same sizes were (issue566801)

All done - thanks, Lex! On 2010/05/26 18:19:26, Lex wrote: LGTM with nits. No need to rereview if you like the suggested changes. These changes will make SOYC a lot easier to understand. http://gwt-code-reviews.appspot.com/566801/diff/1/3 File

[gwt-contrib] Fixes a bug in the compile report dashboard where entries with same sizes were (issue566801)

Reviewers: Lex, Description: Fixes a bug in the compile report dashboard where entries with same sizes were being swallowed. It also now attributes fields to the corresponding packages, and gives additional hints about what is broken down and what isn't. Please review this at

[gwt-contrib] Re: When SoycReportLinker looks for report files, it now tolerates (issue545801)

LGTM http://gwt-code-reviews.appspot.com/545801/diff/1/3 File dev/core/test/com/google/gwt/core/linker/SoycReportLinkerTest.java (right): http://gwt-code-reviews.appspot.com/545801/diff/1/3#newcode80 dev/core/test/com/google/gwt/core/linker/SoycReportLinkerTest.java:80: } This test is good,

[gwt-contrib] Re: Ensure static initialization is atomic (found by findbugs) (issue341802)

LGTM http://gwt-code-reviews.appspot.com/341802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes for crawling:

http://gwt-code-reviews.appspot.com/161801 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Changes for crawling:

On 2010/03/11 16:45:34, kathrin wrote: Hi Amit, after a lengthy discussion with Joel, we decided to get rid of the CrawlableHyperlink widget. The issue is that it doesn't add enough useful functionality, because the app writer still needs to handle the ! when actually navigating the app to a

[gwt-contrib] Changes for crawling:

Reviewers: amtimanjhi_google.com, Description: Changes for crawling: - CrawlableHyperlink - client-side changes to Showcase sample Review at http://gwt-code-reviews.appspot.com/161801 Please review this at http://gwt-code-reviews.appspot.com/163802 Affected files: M

[gwt-contrib] Re: Changes for crawling:

http://gwt-code-reviews.appspot.com/161801/diff/1/3 File samples/showcase/war/Showcase.html (right): http://gwt-code-reviews.appspot.com/161801/diff/1/3#newcode4 Line 4: script language='javascript' The reason I add the title programmatically is because it changes to reflect the widget that is

[gwt-contrib] Changes for crawling:

Reviewers: amitmanjhi, Description: Changes for crawling: - CrawlableHyperlink - client-side changes to Showcase sample Review at http://gwt-code-reviews.appspot.com/161801 Please review this at http://gwt-code-reviews.appspot.com/165801 Affected files: M

[gwt-contrib] Changes for crawling:

Reviewers: amitmanjhi, Description: Changes for crawling: - CrawlableHyperlink - client-side changes to Showcase sample Please review this at http://gwt-code-reviews.appspot.com/161801 Affected files: M samples/showcase/src/com/google/gwt/sample/showcase/client/Showcase.java M

[gwt-contrib] Re: URI-escape cookies (addresses external issue 4365)

All done, thanks. Committed at r7354. On 2009/12/27 03:29:04, Dan Rice wrote: LGTM with minor nits. http://gwt-code-reviews.appspot.com/128801/diff/1/3 File user/src/com/google/gwt/user/client/Cookies.java (right): http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode112 Line 112:

[gwt-contrib] Fix two CompileReport XML issues

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? It fixes two XML issues in the CompileReport: 1) There was a bug in the escapeXML method that would let all special characters after the first one slip through. This is now fixed. 2) Because of the difference in encoding

[gwt-contrib] Last chance SOYC-Compile Report cleanup

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? I replaced several user-facing mentions of SOYC with Compile Report to achieve more consistency, especially for people that don't yet know what SOYC is. Thanks, kathrin Please review this at

[gwt-contrib] Make compiler log start counting permutations at 0 (again)

Reviewers: scottb, Description: Scott, could you review this simple patch for me? It reverts an earlier change, so that the compiler log starts counting permutations at 0 again. While less pretty, it's consistent with everything else. Thanks, kathrin Please review this at

[gwt-contrib] Re: Make compiler log start counting permutations at 0 (again)

On 2009/11/23 16:40:11, scottb wrote: LVGTM Thanks, committed to trunk at r7115. http://gwt-code-reviews.appspot.com/112803 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Add deprecation warning to SoycDashboard

Reviewers: Lex, Description: Users should no longer use the SoycDashboard directly, because the compiler option -compileReport will already cause all compile report files to be written to the extras directory. This patch adds a simple warning to users of the dashboard. For people who use -soyc,

[gwt-contrib] Make compile report handle collapsed permutations (rather than swallowing them)

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? The dashboard now enumerates all permutation descriptions, even when two permutations collapse into one. Thanks, kathrin Please review this at http://gwt-code-reviews.appspot.com/110801 Affected files:

[gwt-contrib] Updates to SOYC Test to reflect changes in output directory and 1-based counting

Reviewers: Lex, Description: Hi Lex, the previous two patches have implications for SoycTest, which needed to be updated slightly. This patch does that. Could you review it for me? Thanks, kathrin Please review this at http://gwt-code-reviews.appspot.com/105801 Affected files:

[gwt-contrib] Yet another SOYC dashboard styling pass

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? It makes a number of changes to the SOYC dashboard, as outlined by John Skidgel in his mock-up. Thanks, kathrin Please review this at http://gwt-code-reviews.appspot.com/91805 Affected files:

[gwt-contrib] Re: fixes an overly long filename in a compile report

On 2009/10/29 19:52:18, Lex wrote: LTGM. Thanks for taking care of this! http://gwt-code-reviews.appspot.com/90801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---

[gwt-contrib] Clean up SOYC options

Reviewers: Lex, Description: Hi Lex, could you review this small patch for me? It cleans up the SOYC options (b/2153332) by: 1) undocumenting -soyc and introducing -compileReport (which is documented), both of which will set soycEnabled to true. 2) causing -XsoycDetailed to turn on soycEnabled

[gwt-contrib] test case for -soyc and SOYC dashboard

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? It implements a simple test case that invokes the compiler with the -soyc flag, produces the dashboard files, and then checks whether the files exist. Getting this to work also required some build file magic (thanks to

[gwt-contrib] right-click for tree items

Reviewers: jlabanca, Description: Hi John, could you review this patch for me? It's very small. The problem we were seeing was that for tree items, right- and middle-clicks were handled and the associated event were fired. This patch checks that only left-clicks fire such events. Thanks!

[gwt-contrib] Re: changes to hyperlink and Showcase sample

Thanks to both of you! Committed to branch at r6278. http://gwt-code-reviews.appspot.com/74801/diff/1002/1006 File samples/showcase/src/com/google/gwt/sample/showcase/server/CrawlServlet.java (right): http://gwt-code-reviews.appspot.com/74801/diff/1002/1006#newcode37 Line 37: * Servlet that

[gwt-contrib] changes to hyperlink and Showcase sample

Reviewers: Dan Rice, Description: Hi Dan, could you review this patch for me? It implements changes to the Showcase sample and to hyperlink. 1) Showcase: Showcase needs to have indexable hyperlinks (see below) and needs changes to web.xml as well as an added CrawlServlet that invokes the

[gwt-contrib] overhaul of styling in SOYC dashboard

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? This patch overhauls styling in the SOYC dashboard. It aims at following styling guidelines by Google, including the color scheme, which is modeled after trends.google.com. It does this by getting rid of most of the

[gwt-contrib] Re: Simple crawler

Thanks, committed to crawlability branch at r6204. http://gwt-code-reviews.appspot.com/70802/diff/24/28 File tools/simple-crawler/src/com/google/gwt/simplecrawler/Settings.java (right): http://gwt-code-reviews.appspot.com/70802/diff/24/28#newcode102 Line 102: * Processes the arguments from the

[gwt-contrib] Simple crawler

Reviewers: Dan Rice, Description: Could you review this patch for me? This simple crawler is intended to be used by users who crawl-enable their apps. It gives them an idea of what the crawler will see, without making any guarantees about emulating the real google crawler. It takes in either

[gwt-contrib] Re: Add permutation info to index page for each permutation

On 2009/09/10 15:42:26, Lex wrote: LGTM Thanks, committed at r6138. http://gwt-code-reviews.appspot.com/64808 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---

[gwt-contrib] Add permutation info to index page for each permutation

Reviewers: , Description: Hi Lex, could you review this patch for me? It follows up on a (very useful!) feature request by Adam to add permutation information to the index page for each individual permutation. Thanks, kathrin Please review this at http://gwt-code-reviews.appspot.com/64808

[gwt-contrib] Re: Eliminate dead links in SOYC dashboard when displayDependencies is off

On 2009/09/08 15:05:18, Lex wrote: LGTM. Good catch! Thanks, committed at r6096. http://gwt-code-reviews.appspot.com/65803 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change SOYC dashboard to accept symbol maps created by different linker

On 2009/09/03 16:14:54, kathrin wrote: Thanks, Lex, for a desk review. Committed at r6098. http://gwt-code-reviews.appspot.com/65802 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Change SOYC dashboard to accept symbol maps created by different linker

Reviewers: Lex, schuck_google.com, Description: This change to the dashboard updates the settings parser to read symbol maps files created with the CompactSymbolLinker. Please review this at http://gwt-code-reviews.appspot.com/65802 Affected files:

[gwt-contrib] Eliminate dead links in SOYC dashboard when displayDependencies is off

Reviewers: Lex, Description: Hi Lex, could you review this patch for me? It eliminates some dead links in the dashboard when displayDependencies is turned off. Thanks! kathrin Please review this at http://gwt-code-reviews.appspot.com/65803 Affected files:

[gwt-contrib] Fix path issue in SoycDashboard after moving soyc resources to a sub-directory

Reviewers: Lex, Description: Hi Lex, could you review this tiny patch for me? It fixes a problem we had introduced by moving all SOYC resources to a subdirectory: when people were using the -resources flag to actually point at the directory that contains the css and image files, the dashboard

[gwt-contrib] Re: Call webClient.closeAllWindows() in htmlunit code

Hi Amit, I think this is the right direction! But I do think there's still a problem here, if I read the HtmlUnit documentation correctly. It says: - waitForBackgroundJavaScriptStartingBefore This method blocks until all background JavaScript tasks scheduled to start executing

[gwt-contrib] Re: Call webClient.closeAllWindows() in htmlunit code

In a discussion with Amit and Joel, we've determined that the tests that this will apply to do not use History tokens, so the repeated timeout won't apply. We couldn't think of any other cases where there would be lingering JavaScript jobs. TODO - we should revisit this issue at some point and

[gwt-contrib] Re: Illegal XML characters in SOYC XML files

Thanks, Lex. I didn't try the #x; idea (see Ian's comment), but I also added the other illegal characters. I'll leave the recoverability (in the dashboard) for another day: (x00) and (u) seem good to me for human consumption, and the surrogate blocks characters shouldn't really ever in an

[gwt-contrib] Illegal XML characters in SOYC XML files

Reviewers: Lex, Description: Hi Lex, would you review this simple patch for me? It catches the illegal XML characters that were throwing off the SAX parser. TODO: there are still some character that could theoretically sneak into our xml files, see http://www.w3.org/TR/REC-xml/#charsets.

[gwt-contrib] clean up compiler logging output

permutation 4... Compiling permutation 5... Compiling permutation 6... Compile of permutations succeeded Linking into /Users/kprobst/work/gwt-trunk-soyc/eclipse/samples/Hello/war/hello Link succeeded Compilation succeeded -- 17.314s Please review this at http://gwt-code

[gwt-contrib] Re: move SOYC resources out of the top-level jar directory

LGTM http://gwt-code-reviews.appspot.com/60803/diff/1/2 File tools/soyc-vis/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java (right): http://gwt-code-reviews.appspot.com/60803/diff/1/2#newcode126 Line 126: + /resources/; Just wondering why you infer the path here (you hard-code it in the

[gwt-contrib] Re: SOYC for multiple permutations, plus updated layout

Thanks for the feedback! I've fixed the following in the updated patch: 1) backwards compatibility 2) make GlobalInformation contain no static fields. GlobalInformation is now re-created for each iteration. Truly global information (e.g., permutation information strings) are now moved to

[gwt-contrib] SOYC for multiple permutations, plus updated layout

Reviewers: Lex, Description: Hi Lex, would you review this patch to the SOYC dashboard for me? It has two main parts: 1) SOYC for multiple permutations. The dashboard will now read all files in the symbolMaps directory and from it derive all the permutations and their attributes. The

[gwt-contrib] Re: faster size breakdown with non-fractional billing

-reviews.appspot.com/51804/diff/1/18 File dev/core/src/com/google/gwt/dev/js/JsReportGenerationVisitor.java (right): http://gwt-code-reviews.appspot.com/51804/diff/1/18#newcode39 Line 39: super(out, map); (note kprobst) - do we now always need to pass in the map? does this make sense without soyc? http://gwt

[gwt-contrib] Re: SOYC filenames all end with .html

LGTM http://gwt-code-reviews.appspot.com/54813 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---

[gwt-contrib] Re: SOYC generates more dependency trees

On 2009/07/01 17:56:26, Lex wrote: Generally, LGTM. I had a few comments below. Going forward, I notice that SOYC + the dashboard are growing in complexity and functionality - there is nothing wrong with that, and it's definitely a good step forward. However, would it make sense to write up

[gwt-contrib] Manifest for SOYC permutations

Reviewers: Lex, Description: This patch creates a manifest file for each compilation permutation if the compilation is SOYC enabled. The manifest files are stored alongside other SOYC manifests (in the soycReport directory). Each manifest file contains all non-gwt properties for the

[gwt-contrib] Re: SOYC gives drill-down on size for leftover and exclusive fragments

LGTM The actual patch looks good to me (see only two small comments below), but I am a little concerned about the unescaping of XML. Please see my comment below. http://gwt-code-reviews.appspot.com/33843/diff/1/6 File tools/soyc-vis/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java (right):

[gwt-contrib] Re: Speedups for -soyc compilation

LGTM I do want to re-iterate Lex's point that we need to make sure that the dashboard does not produce different results with this. This is especially true because we know that the output of the SOYC files has changed with this patch. Having said that, this should really speed up compilation,

[gwt-contrib] Re: Speed up escapeXml methods

LGTM Like you mentioned, with a more intrusive change we might (in the future) be able to get away without ever converting to/from String. http://gwt-code-reviews.appspot.com/34814 --~--~-~--~~~---~--~~

[gwt-contrib] Re: add -out to SoycDashboard

On 2009/05/18 21:56:02, spoon wrote: LGTM http://gwt-code-reviews.appspot.com/33825 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---

[gwt-contrib] Addresses issues 1820736 and 1843669 (Soyc dashboard improvements)

Reviewers: spoon, Please review this at http://gwt-code-reviews.appspot.com/33822 Affected files: tools/soyc-vis/src/com/google/gwt/soyc/GlobalInformation.java tools/soyc-vis/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java tools/soyc-vis/src/com/google/gwt/soyc/Settings.java