Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345673004:
Updating blimp_shell doc (Closed)
|
Created:
4 years, 3 months ago by scf Modified:
4 years, 3 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdating blimp_shell doc
Updating instructions for the blimp shell
BUG=
Committed: https://crrev.com/c960f497cd1498aa600423b20a6654421b444b27
Cr-Commit-Position: refs/heads/master@{#419201}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressing comments #
Total comments: 1
Patch Set 3 : addressing comments #Patch Set 4 : missed the 80 line character limit #Messages
Total messages: 17 (6 generated)
Description was changed from ========== Updating blimp_shell doc Updating instructions for the blimp shell BUG= ========== to ========== Updating blimp_shell doc Updating instructions for the blimp shell BUG= ==========
scf@chromium.org changed reviewers: + steimel@chromium.org, wez@chromium.org
Thanks for cleaning this up :) https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md File blimp/docs/running.md (right): https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md#newco... blimp/docs/running.md:156: shipped. The Linux client is not built as part of the `blimp` target. It requires x11 which is not defined in the engine. nit: Let's rephrase this to just "The Linux client is useful for development purposes where the full Android UI is not required.", and fix the line length. We have a handy tool "git cl format" which deals with stuff line line-length limits, but I'm not sure it checks .md files. https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md#newco... blimp/docs/running.md:161: echo "" > out-linux/Client/args.gn Do you need this? Shouldn't "gn gen out-linux/Client" do the right thing? https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md#newco... blimp/docs/running.md:181: **PS:** The `/tmp/blimpengine-token` is a file any sequence of characters. Just make sure when you start the engine, you use the same file. Let's be explicit on the requirements here; is an empty file sufficient, or should the developer e.g: echo "dummy_token" > token_file ?
thanks for the feedback. will clean this up https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md File blimp/docs/running.md (right): https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md#newco... blimp/docs/running.md:156: shipped. The Linux client is not built as part of the `blimp` target. It requires x11 which is not defined in the engine. On 2016/09/16 00:12:38, Wez wrote: > nit: Let's rephrase this to just "The Linux client is useful for development > purposes where the full Android UI is not required.", and fix the line length. > > We have a handy tool "git cl format" which deals with stuff line line-length > limits, but I'm not sure it checks .md files. Acknowledged. https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md#newco... blimp/docs/running.md:161: echo "" > out-linux/Client/args.gn On 2016/09/16 00:12:38, Wez wrote: > Do you need this? Shouldn't "gn gen out-linux/Client" do the right thing? Acknowledged. https://codereview.chromium.org/2345673004/diff/1/blimp/docs/running.md#newco... blimp/docs/running.md:181: **PS:** The `/tmp/blimpengine-token` is a file any sequence of characters. Just make sure when you start the engine, you use the same file. On 2016/09/16 00:12:38, Wez wrote: > Let's be explicit on the requirements here; is an empty file sufficient, or > should the developer e.g: > > echo "dummy_token" > token_file > > ? Acknowledged.
thanks for the feedback. will clean this up
LGTM w/ nit https://codereview.chromium.org/2345673004/diff/20001/blimp/docs/running.md File blimp/docs/running.md (right): https://codereview.chromium.org/2345673004/diff/20001/blimp/docs/running.md#n... blimp/docs/running.md:160: mkdir -p out-linux/Client nit: Looks like gn gen will take care of this :)
updated with comments
sorry missed the 80 line character limit
The CQ bit was checked by wez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2345673004/#ps60001 (title: "missed the 80 line character limit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Updating blimp_shell doc Updating instructions for the blimp shell BUG= ========== to ========== Updating blimp_shell doc Updating instructions for the blimp shell BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Updating blimp_shell doc Updating instructions for the blimp shell BUG= ========== to ========== Updating blimp_shell doc Updating instructions for the blimp shell BUG= Committed: https://crrev.com/c960f497cd1498aa600423b20a6654421b444b27 Cr-Commit-Position: refs/heads/master@{#419201} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c960f497cd1498aa600423b20a6654421b444b27 Cr-Commit-Position: refs/heads/master@{#419201} |
