feat(import): add browser data migration - #50
Conversation
📝 WalkthroughWalkthroughThis PR adds a standalone ChangesBrowser Data Migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ImportUI as dao://import
participant WebUIHandler as DaoImportUIHandler
participant MigrationService as DaoMigrationService
participant SourceAdapter as DaoChromiumProfileAdapter
participant MigrationTarget as DaoChromiumMigrationTarget
User->>ImportUI: Open import page
ImportUI->>WebUIHandler: detectImportSources
WebUIHandler->>MigrationService: DetectSources
MigrationService-->>WebUIHandler: Source profiles
WebUIHandler-->>ImportUI: Sources
User->>ImportUI: Select source and categories
ImportUI->>WebUIHandler: startBrowserMigration
WebUIHandler->>MigrationService: Start(sourceId, categories)
MigrationService->>SourceAdapter: Read category records from snapshot
SourceAdapter-->>MigrationService: Records
MigrationService->>MigrationTarget: Write bookmarks/history/passwords/tabs/extensions
MigrationTarget-->>MigrationService: Write results
MigrationService-->>WebUIHandler: State updates
WebUIHandler-->>ImportUI: browser-migration-state-changed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e0ae719 to
2dd29fc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (29)
src/dao/browser/ui/dao_ui_sources.gni (1)
208-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the GN lists sorted.
//dao/browser/ui/webui/resources/import:resourcesis placed after:sidebar, and the new unittest entries are placed before the existingagententry.gn formatsorts contiguous string lists, so these insertions cause reformat churn.Also applies to: 215-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/ui/dao_ui_sources.gni` around lines 208 - 211, Sort the GN resource lists in dao_ui_sources.gni lexicographically, placing import before sidebar and moving the unittest entries after the existing agent entry as appropriate. Keep each contiguous string list consistently ordered so it matches gn format output.src/patches/chrome/browser/ui/webui/chrome_web_ui_configs.cc.patch (1)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the disabled upstream registration instead of commenting it out.
The
skills::SkillsUIConfigline stays in the patch as a comment. The explanatory comment above it already records the reason. Removing the commented statement keeps the patch smaller and reduces conflicts on Chromium uprevs. If the include for the upstream config is now unused, remove it as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patches/chrome/browser/ui/webui/chrome_web_ui_configs.cc.patch` around lines 38 - 42, Remove the commented-out skills::SkillsUIConfig registration from the WebUI configuration patch, keeping the existing explanatory comment about DaoSkillsUIConfig. Check whether the upstream SkillsUIConfig include is now unused and remove it if so.src/dao/browser/ui/webui/resources/import/dao_import_app.ts (1)
646-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sourceInitial_always returns an empty string.The only call site is line 859, which passes
''. The fallback glyph therefore renders nothing when the source is unknown. Either pass the real kind, or remove the helper and the fallback branch.Also applies to: 856-860
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/ui/webui/resources/import/dao_import_app.ts` around lines 646 - 648, Fix the unknown-source fallback in sourceInitial_ and its call site so it receives the actual source kind instead of the empty string, ensuring the fallback glyph is non-empty. Preserve the existing fallback rendering behavior and remove the helper only if the real kind cannot be passed through.src/dao/browser/ui/webui/resources/import/__tests__/dao_import_app.test.ts (1)
279-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot fail.
jsdom does not resolve author stylesheet declarations for
min-heighton the fixture, sogetComputedStyle(completedSegment).minHeightreturns an empty string orauto. The assertion at line 302 passes even if.completion { min-height: 390px; }were changed to match.rail span. The test provides no regression protection.Assert on the stylesheet text instead, because that is deterministic without a layout engine.
💚 Proposed replacement assertion
- const style = document.createElement('style'); - style.textContent = cssText; - const fixture = document.createElement('div'); - fixture.innerHTML = ` - <div class="shell"> - <header><div class="rail"><span class="done"></span></div></header> - </div>`; - document.body.append(style, fixture); - - const completedSegment = fixture.querySelector('.rail span')!; - expect(getComputedStyle(completedSegment).minHeight).not.toBe('390px'); + const minHeightRules = + [...cssText.matchAll(/([^{}]+)\{[^}]*min-height:\s*390px/g)] + .map(match => match[1]!.trim()); + expect(minHeightRules).toEqual(['.completion']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/ui/webui/resources/import/__tests__/dao_import_app.test.ts` around lines 279 - 303, Update the test around createApp and the generated style text to assert directly against cssText, verifying the completion selector does not assign 390px min-height to completed rail segments. Remove the getComputedStyle-based assertion and keep the check deterministic through the stylesheet declaration text.src/dao/browser/ui/webui/resources/import/import_bridge.ts (1)
53-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd timeout cleanup to
sendAsync.Keep the current
FireWebUIListenercallback-event protocol. Do not replace it withsendWithPromise.If the native handler is destroyed before replying,
sendAsyncleaves its promise pending and retains its listener indefinitely. Add timeout and rejection cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/ui/webui/resources/import/import_bridge.ts` around lines 53 - 85, Update sendAsync to reject and remove its callback listener when no native response arrives within a timeout, while preserving the existing callback-event protocol through chrome.send. Ensure successful responses clear the timeout before resolving, and timeout handling removes the listener before rejecting.src/dao/browser/import/dao_migration_job.h (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
#include <cstdint>.
UpdateProgressusesuint64_t, but the header relies on a transitive include fromdao_migration_types.h. The coding guidelines require confirming that all#includedirectives are present.♻️ Proposed change
+#include <cstdint> `#include` <map> `#include` <optional> `#include` <string> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_job.h` around lines 8 - 13, Add the direct <cstdint> include to dao_migration_job.h, alongside the existing standard-library includes, so UpdateProgress can use uint64_t without relying on dao_migration_types.h's transitive includes.Source: Coding guidelines
src/dao/browser/import/dao_legacy_profile_writer.cc (1)
54-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
AddHistoryPageand avoid the temporary vector.The function builds
visitsand then move-inserts it intopending_history_. Append directly topending_history_.♻️ Proposed refactor
void DaoLegacyProfileWriter::AddHistoryPage(const history::URLRows& page, history::VisitSource) { - std::vector<HistoryVisit> visits; - visits.reserve(page.size()); + pending_history_.reserve(pending_history_.size() + page.size()); for (const history::URLRow& row : page) { HistoryVisit visit; visit.url = row.url(); visit.title = row.title(); visit.visit_time = row.last_visit(); - visits.push_back(std::move(visit)); + pending_history_.push_back(std::move(visit)); } - pending_history_.insert(pending_history_.end(), - std::make_move_iterator(visits.begin()), - std::make_move_iterator(visits.end())); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_legacy_profile_writer.cc` around lines 54 - 68, Update DaoLegacyProfileWriter::AddHistoryPage to append each constructed HistoryVisit directly to pending_history_ while iterating over page, removing the temporary visits vector, its reserve call, and the subsequent move-insert.src/dao/browser/import/dao_chromium_migration_target.cc (3)
160-193: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
AddHistoryVisitverifies the write but does not verify the title.
AddPageandSetPageTitleare fire-and-forget, andOnHistoryWriteVerifiedonly matches onvisit_time. A droppedSetPageTitleis reported as a successful import. This is acceptable for a merge-only import, and the verification of the visit row is the important part. No change required unless title fidelity is a stated requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_migration_target.cc` around lines 160 - 193, No code change is required: retain AddHistoryVisit and OnHistoryWriteVerified as-is, since visit persistence is the required verification for this merge-only import and title verification is not required.
278-298: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
IsTabOpencompares the visible URL exactly.
GetVisibleURL()differs from the imported URL after a redirect or a trailing-slash normalization, so a tab that is already open can be imported again as a dormant tab. Consider comparingGetLastCommittedURL()as well, or comparing normalized specs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_migration_target.cc` around lines 278 - 298, Update DaoChromiumMigrationTarget::IsTabOpen to recognize already-open tabs when the imported URL differs due to redirects or trailing-slash normalization. Compare url against each WebContents object's GetLastCommittedURL() in addition to GetVisibleURL(), preserving the existing early-exit behavior when either URL matches.
431-441: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
CancelExtensionInstallsrelies on an invariant that is not enforced.If
extension_installer_is non-null, the code assumesextension_queue_is non-empty and callsfront().StartNextExtensionInstallestablishes that invariant today, andOnExtensionInstallFinishedresets the installer before popping. Add a defensive!extension_queue_.empty()check so a future reordering cannot dereference an empty deque.🛡️ Proposed guard
- if (extension_installer_) { + if (extension_installer_ && !extension_queue_.empty()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_migration_target.cc` around lines 431 - 441, Update DaoChromiumMigrationTarget::CancelExtensionInstalls so the active-entry preservation branch runs only when extension_installer_ is non-null and extension_queue_ is non-empty; otherwise clear the queue as currently done. Keep MaybeFinishExtensionInstalls() invoked after either path.src/dao/browser/import/dao_chromium_profile_adapter.cc (1)
35-43: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ChromiumTimeFromStringrejects valid zero timestamps and silently maps errors to a null time.
microseconds <= 0treats0as invalid.base::Time()is also the value returned for a parse error, so callers cannot distinguish "no date" from "bad data". The current callers only use the value for bookmark creation time, so the impact is small. Usemicroseconds < 0for correctness.♻️ Proposed change
- if (!value || !base::StringToInt64(*value, µseconds) || - microseconds <= 0) { + if (!value || !base::StringToInt64(*value, µseconds) || + microseconds < 0) { return base::Time(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_profile_adapter.cc` around lines 35 - 43, Update ChromiumTimeFromString to reject only negative microsecond values by changing the validation boundary from <= 0 to < 0, allowing zero timestamps to convert through base::Time::FromDeltaSinceWindowsEpoch while preserving existing null-time behavior for missing or unparsable input.src/dao/browser/import/dao_migration_writer_unittest.cc (1)
97-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
QueueExtensionInstallnever simulates a rejected queue.The fake always returns
true, and it also marks the extension as installed. No test covers the failure branch ofWriteExtensions, so a regression that miscounts a rejected queue would pass. Add aaccept_extension_queueflag and a test that expectsfailed == 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_writer_unittest.cc` around lines 97 - 100, Update the fake QueueExtensionInstall implementation to honor an accept_extension_queue flag, returning false without recording the extension when queueing is rejected. Add a WriteExtensions test configuring the flag to reject the queue and assert that failed equals 1.src/dao/browser/import/dao_migration_job.cc (1)
14-21: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA job with an empty category selection is terminal immediately.
std::all_ofreturnstruefor an emptycategory_states_, soIsTerminal()istrueandStartCategoryalways fails. That behavior is probably intended, but it is implicit. Reject an empty selection inDaoMigrationService::Start, or add a comment here that records the intent.Duplicate entries in
selected_categories_also collapse to one map entry whileGetState()still emits oneCategoryStateper vector entry. Deduplicateselected_categories_in the constructor to keep the two views consistent.Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_job.cc` around lines 14 - 21, Update DaoMigrationJob and DaoMigrationService::Start to handle empty selections explicitly, either rejecting them in Start or documenting the intentional immediate-terminal behavior in the constructor. In DaoMigrationJob::DaoMigrationJob, deduplicate selected_categories_ before populating category_states_ so GetState() and the map maintain consistent category counts.src/dao/browser/import/dao_migration_job_unittest.cc (1)
109-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining guard branches of the state machine.
The suite does not cover these rejections, and each one protects an invariant that the service depends on:
StartCategorywhile another category is running.StartCategoryafterRequestCancel().CancelRunningCategoryAtBatchBoundary()without a priorRequestCancel()(must returnfalse).UpdateProgresswithtotal_items == 0, which must setindeterminatetotrue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_job_unittest.cc` around lines 109 - 131, Add focused tests to DaoMigrationJobTest covering the missing state-machine guards: reject StartCategory when another category is running and after RequestCancel(), verify CancelRunningCategoryAtBatchBoundary returns false without cancellation being requested, and verify UpdateProgress with total_items equal to zero sets indeterminate to true. Reuse the existing TestSource, categories, and state assertions used by CancelsPendingWorkAtSafeBatchBoundary.src/dao/browser/import/dao_migration_service.cc (3)
684-692: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value第 687 行缺少
target_判空。同一文件的第 249 行与第 768 行都在使用
target_前判空。这里直接解引用。Shutdown()第 291 行会重置target_，此后若仍有已投递的WriteNextBatch任务执行，将出现空指针解引用。弱指针已在Shutdown()中失效，因此当前不可达；为保持一致性，建议补上判空。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service.cc` around lines 684 - 692, 在处理 DataCategory::kExtensions 的分支中，调用 DaoMigrationService::target_->FinishExtensionInstalls 前先判空 target_；若 target_ 已被 Shutdown() 重置，则直接返回并保持现有流程不变。
511-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value第 517 行在
item为 0 时会让分类永久停留在 pending。
LegacyImportItem对kTabs与kExtensions返回NONE。此时循环直接返回，既不启动也不失败该分类，任务无法进入终态。当前Start已按supported_categories过滤，所以该路径不可达，但契约一旦变化就会造成任务卡住。建议对不支持的分类显式失败，而不是静默返回。
♻️ 建议的防御性修复
const uint16_t item = LegacyImportItem(category); - if (!item || !job_->StartCategory(category)) { + if (!item) { + if (job_->StartCategory(category)) { + job_->FailCategory(category, "category_unsupported"); + NotifyObservers(); + } + continue; + } + if (!job_->StartCategory(category)) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service.cc` around lines 511 - 519, Update the category handling in the migration loop around LegacyImportItem and StartCategory so an item value of 0 explicitly marks the category as failed before returning, rather than leaving it pending. Preserve the existing StartCategory failure behavior for valid legacy items, and use the existing category failure mechanism.
158-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win计数阶段的快照无法被取消。
第 160 行创建的取消标记只保存在局部
request中，没有赋值给snapshot_cancellation_。Cancel()第 237-239 行只取消成员标记，因此计数阶段的标签页快照复制会继续运行到结束。建议保存到成员变量，使计数与迁移共用同一取消路径。
♻️ 建议的修复
if (category == DataCategory::kTabs) { + snapshot_cancellation_ = base::MakeRefCounted<SnapshotCancellationFlag>(); SnapshotRequest request = BuildSnapshotRequest(category, *profile_path); - request.cancellation = base::MakeRefCounted<SnapshotCancellationFlag>(); DaoProfileSnapshot::Create(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service.cc` around lines 158 - 167, 在 DaoMigrationService 中更新 kTabs 的计数快照创建逻辑，将创建的 SnapshotCancellationFlag 同时保存到成员变量 snapshot_cancellation_，而不是仅保留在局部 request.cancellation 中。确保 Cancel() 能通过该成员取消计数阶段的标签页快照，并与迁移阶段共用同一取消路径。src/dao/browser/import/dao_migration_service.h (1)
8-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value补齐
<utility>与base/files/file_path.h。第 142 行使用
std::pair，第 95 与 97 行使用base::FilePath。请显式包含对应头文件。依据编码指南："Before considering the task complete, confirm that all
#includedirectives are present."♻️ 建议的 include 调整
`#include` <string> +#include <utility> `#include` <variant> `#include` <vector> `#include` "base/callback_list.h" +#include "base/files/file_path.h" `#include` "base/memory/raw_ptr.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service.h` around lines 8 - 27, 在 dao_migration_service.h 的 include 区域显式添加 <utility> 和 base/files/file_path.h，以覆盖 std::pair 与 base::FilePath 的直接依赖；保留现有包含并按项目头文件排序规范排列。Source: Coding guidelines
src/dao/browser/import/dao_chromium_password_decryptor_mac.h (1)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议补齐直接使用的头文件。
本文件直接使用
base::span、std::u16string与uint8_t，但这些均依赖dao_source_adapter.h的传递引入。请显式包含base/containers/span.h、<string>与<cstdint>。依据编码指南："Before considering the task complete, confirm that all
#includedirectives are present."♻️ 建议的 include 调整
`#include` <array> +#include <cstdint> `#include` <optional> +#include <string> +#include "base/containers/span.h" `#include` "dao/browser/import/dao_migration_types.h" `#include` "dao/browser/import/dao_source_adapter.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_password_decryptor_mac.h` around lines 8 - 12, 补齐头文件的直接依赖：在 dao_chromium_password_decryptor_mac.h 中显式包含 base/containers/span.h、<string> 和 <cstdint>，以支持文件直接使用的 base::span、std::u16string 与 uint8_t；保留现有 include，并不要依赖 dao_source_adapter.h 的传递引入。Source: Coding guidelines
src/dao/browser/import/dao_migration_service_factory.h (2)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value补齐
<memory>。第 28 行使用
std::unique_ptr，但本文件未包含<memory>。依据编码指南："Before considering the task complete, confirm that all
#includedirectives are present."♻️ 建议的 include 调整
+#include <memory> + `#include` "base/no_destructor.h" `#include` "components/keyed_service/content/browser_context_keyed_service_factory.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service_factory.h` around lines 8 - 9, 在头文件的 include 区域补充标准库头文件 <memory>，以便声明或使用 std::unique_ptr 的代码具备直接依赖；保留现有 include 不变，并确认该文件的所有 include 依赖完整。Source: Coding guidelines
17-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value改用
ProfileKeyedServiceFactory管理 profile 选择。Chromium
149.0.7827.201提供此基类。当前服务仅用于 regular profile 时，可使用默认的ProfileKeyedServiceFactory("DaoMigrationService")，并删除手写的GetBrowserContextToUse。如果 guest 或 incognito 需要不同策略，请通过ProfileSelections显式配置。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service_factory.h` around lines 17 - 31, 将 DaoMigrationServiceFactory 改为继承 ProfileKeyedServiceFactory，并使用默认的 “DaoMigrationService” 配置管理 regular profile；删除 GetBrowserContextToUse 声明及其实现，保留现有服务构造和获取接口。若代码支持 guest 或 incognito，改用 ProfileSelections 显式指定策略。src/dao/browser/import/dao_chromium_profile_adapter_unittest.cc (1)
7-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value补齐测试直接使用的头文件。
第 24 行使用
std::optional，第 48 行使用std::vector，第 114 行使用base::as_byte_span。请显式包含对应头文件，不要依赖传递引入。依据编码指南："Before considering the task complete, confirm that all
#includedirectives are present."♻️ 建议的 include 调整
+#include <optional> `#include` <string> `#include` <string_view> `#include` <utility> +#include <vector> +#include "base/containers/span.h" `#include` "base/files/file_util.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_profile_adapter_unittest.cc` around lines 7 - 17, 在测试文件的 include 区域补充直接使用符号所需的头文件：为 std::optional、std::vector 和 base::as_byte_span 分别显式引入对应声明，保留现有 include，并避免依赖传递引入。Source: Coding guidelines
src/dao/browser/import/dao_legacy_profile_writer.h (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value补齐
<vector>与<cstddef>。第 34、47、48 行使用
std::vector，第 49 行使用size_t，但本文件未直接包含对应头文件。依据编码指南："Before considering the task complete, confirm that all
#includedirectives are present."♻️ 建议的 include 调整
+#include <cstddef> `#include` <string> +#include <vector> `#include` "base/functional/callback.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_legacy_profile_writer.h` around lines 8 - 14, 在 import 相关声明中补充直接依赖的 <vector> 和 <cstddef> 头文件，确保 dao_legacy_profile_writer.h 中使用的 std::vector 与 size_t 可独立编译；保留现有 include，并按项目规范整理新增 include。Source: Coding guidelines
src/dao/browser/import/dao_chromium_profile_adapter.h (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value同一 PR 内混用了两种 C++ 格式风格。 部分新文件使用 LLVM 风格（访问说明符不缩进、
Type &name、switch case 不缩进），而src/dao/browser/import/dao_migration_writer.h与src/dao/browser/import/dao_migration_writer.cc使用 Chromium 风格（public:、Type& name、case 缩进）。根因是这些文件未按仓库的 Chromium clang-format 配置格式化。请统一为 Chromium 风格。
src/dao/browser/import/dao_chromium_profile_adapter.h#L19-L25：将public:/private:缩进一格，并把DaoChromiumProfileAdapter &改为DaoChromiumProfileAdapter&形式。src/dao/browser/import/dao_migration_service.h#L42-L49：将public:/private:缩进一格，并把const DaoMigrationService &等引用与指针的&/*靠左绑定到类型。src/dao/browser/import/dao_migration_service.cc#L385-L401：将switch内的case标签缩进两格，并把参数中的Type &name/Type *name改为Type& name/Type* name。依据编码指南："Follow Chromium C++ style, including
raw_ptr<>,METADATA_HEADER, include guards, and existingbase/viewsownership conventions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_profile_adapter.h` around lines 19 - 25, 统一按仓库 Chromium clang-format 风格格式化以下位置：src/dao/browser/import/dao_chromium_profile_adapter.h:19-25 将访问说明符缩进一格并采用类型紧邻引用符的写法；src/dao/browser/import/dao_migration_service.h:42-49 同样调整访问说明符及引用、指针绑定方式；src/dao/browser/import/dao_migration_service.cc:385-401 将 switch 的 case 标签缩进两格，并统一参数中的引用和指针格式。Sources: Coding guidelines, Learnings
src/dao/browser/import/dao_profile_snapshot.cc (1)
201-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议使用
FILE_PATH_LITERAL拼接侧车路径。
source.value()的类型是base::FilePath::StringType。在 POSIX 上它是std::string，与std::string(suffix)相加可以编译；在 Windows 上它是std::wstring，该拼接无法编译。当前默认根路径只覆盖 macOS，但用FILE_PATH_LITERAL可以避免后续移植时的编译错误。♻️ 建议重构
- for (std::string_view suffix : {"-wal", "-shm"}) { + for (const base::FilePath::CharType* suffix : + {FILE_PATH_LITERAL("-wal"), FILE_PATH_LITERAL("-shm")}) { outcome = CopyStableFile( - base::FilePath(source.value() + std::string(suffix)), - base::FilePath(destination.value() + std::string(suffix)), + base::FilePath(source.value() + suffix), + base::FilePath(destination.value() + suffix), request.max_attempts, request.cancellation.get(), false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_profile_snapshot.cc` around lines 201 - 205, Update the sidecar-path construction in the loop around CopyStableFile to use FILE_PATH_LITERAL-compatible path components instead of std::string(suffix), preserving the "-wal" and "-shm" suffixes while supporting both POSIX and Windows FilePath::StringType.src/dao/browser/import/dao_source_detector.h (1)
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议增加
SEQUENCE_CHECKER保护成员状态。
Detect、OnDetectionComplete与ResolveProfilePath都读写profile_paths_和detection_generation_，而这些方法只在调用序列上安全。加入 sequence checker 可以在调试构建中及早发现误用。♻️ 建议重构
+#include "base/sequence_checker.h" ... std::map<std::string, base::FilePath> profile_paths_; uint64_t detection_generation_ = 0; + SEQUENCE_CHECKER(sequence_checker_); base::WeakPtrFactory<DaoSourceDetector> weak_ptr_factory_{this};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_source_detector.h` around lines 59 - 69, 为 DaoSourceDetector 增加 SEQUENCE_CHECKER 成员，保护仅在调用序列上访问的状态。 在 Detect、OnDetectionComplete 和 ResolveProfilePath 中执行序列检查，覆盖对 profile_paths_ 与 detection_generation_ 的读写；同时补充所需头文件并保持现有逻辑不变。scripts/commands/__tests__/browser_import_contract.test.ts (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value该正则与 clang-format 的换行结果强耦合。
正则要求
CreateSequencedTaskRunner(之后紧跟{base::MayBlock(),。dao_profile_snapshot.cc目前正好是这种排列，测试可以通过。如果之后 trait 顺序调整（例如把base::TaskPriority放在首位）或格式化插入注释，测试会出现与所有权契约无关的假失败。建议只断言契约要点：同一次
CreateSequencedTaskRunner调用中出现MayBlock与BLOCK_SHUTDOWN。♻️ 建议重构
- expect(implementation).toMatch( - /CreateSequencedTaskRunner\(\s*\{base::MayBlock\(\),[\s\S]*?base::TaskShutdownBehavior::BLOCK_SHUTDOWN\}/, - ); + expect(implementation).toMatch( + /CreateSequencedTaskRunner\([^)]*base::MayBlock\(\)[^)]*base::TaskShutdownBehavior::BLOCK_SHUTDOWN/, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/commands/__tests__/browser_import_contract.test.ts` around lines 22 - 24, Relax the regex assertion in the browser import contract test so it validates that the same CreateSequencedTaskRunner call contains both base::MayBlock() and base::TaskShutdownBehavior::BLOCK_SHUTDOWN, without depending on trait ordering, whitespace, line breaks, or comments.src/dao/browser/import/dao_chromium_password_decryptor_mac.mm (1)
88-98: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win使用
crypto::SecureZeroBuffer清零敏感缓冲区。
FindGenericPassword返回的std::vector<uint8_t>不会自动清零。请在LoadKey()返回前清零password和derived_key，并在析构函数中清零已设置的key_。使用crypto::SecureZeroBuffer(base::span<uint8_t>)，不要调用普通memset。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_chromium_password_decryptor_mac.mm` around lines 88 - 98, Update LoadKey() to use crypto::SecureZeroBuffer(base::span<uint8_t>) to clear the sensitive password buffer and derived_key before every return, including failure paths. Add equivalent secure clearing of the initialized key_ in the decryptor’s destructor, avoiding ordinary memset and preserving the existing key derivation behavior.src/dao/browser/import/dao_migration_service_factory.cc (1)
24-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win补充所有实际使用的 keyed service 依赖。
在构造函数中声明
BookmarkModelFactory、HistoryServiceFactory、ProfilePasswordStoreFactory、extensions::ExtensionRegistryFactory和extensions::ExtensionRegistrarFactory的DependsOn。ProfileBrowserCollection不是 keyed service，不应加入DependsOn。这样可以约束迁移服务及其目标对象的生命周期顺序。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dao/browser/import/dao_migration_service_factory.cc` around lines 24 - 27, 在 DaoMigrationServiceFactory::DaoMigrationServiceFactory 构造函数中，为 BookmarkModelFactory、HistoryServiceFactory、ProfilePasswordStoreFactory、extensions::ExtensionRegistryFactory 和 extensions::ExtensionRegistrarFactory 调用 DependsOn 进行依赖声明；不要将非 keyed service 的 ProfileBrowserCollection 加入依赖列表。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-qa.md`:
- Around line 5-12: Replace the machine-local screenshot references in the
design-qa report with repository-relative artifact paths or supported QA
attachment references. Update both the user-reported broken-state path and the
implementation-evidence comparison path, while preserving the documented
screenshot context.
In `@docs/features.md`:
- Around line 380-389: Clarify the “Merge-only destination writes” documentation
to state that extensions already installed in the destination are skipped. Keep
the “Passwords and extensions” description explicit that compatible source
web-store extensions remain eligible for sequential reinstallation.
In `@src/dao/browser/import/dao_chromium_migration_target.cc`:
- Around line 300-315: Update EnsureImportedTabFolder and the pending-folder
handling to prevent stale pointers after LoadFolderData or folder_items_
reallocation. Reject a second EnsureImportedTabFolder call while a folder is
pending, and clear pending_folder_tab_ids_ when starting a new valid import;
ensure the existing pending-folder state remains consistent for
AbortImportedTabFolder.
- Around line 506-510: Update DaoChromiumMigrationTarget::BookmarkKey to append
an actual NUL separator between entry.title and entry.url.spec(), using an
operation that preserves the byte rather than the NUL-terminated "\0" literal.
Keep the existing JoinPath and key components unchanged.
In `@src/dao/browser/import/dao_chromium_profile_adapter.cc`:
- Around line 144-148: Update ReadHistory and ReadPasswords to open their source
databases using sql::DatabaseOptions().set_read_only(true), matching
CountCandidates. Preserve the existing failure handling and database paths while
preventing read paths from opening the files read-write.
- Around line 194-199: Update the password-reading loop around
password_decryptor_->Decrypt so undecryptable rows after the first are skipped
and counted as failures while successfully decrypted credentials are retained.
Preserve the existing password_decryption_denied batch failure only when
decryption of the first row fails, and return the remaining credentials with the
appropriate failure count.
In `@src/dao/browser/import/dao_migration_service.cc`:
- Around line 623-631: 在 DaoMigrationService 的 PostTaskAndReplyWithResult 调用前，将
snapshot.path 复制到局部变量，并让 ReadSnapshot 使用该副本；随后继续移动 snapshot 给
OnReadComplete，避免同一调用中 snapshot.path 与 std::move(snapshot) 的未定义求值顺序。
In `@src/dao/browser/import/dao_migration_writer.cc`:
- Around line 25-51: Update WriteNextHistory and the corresponding
WriteNextPassword flow so synchronous AddHistoryVisit or AddPassword callbacks
do not recursively re-enter the writer; defer continuation through
SequencedTaskRunner::GetCurrentDefault()->PostTask(), or otherwise use bounded
batches, while preserving result accounting and completion behavior for large
pending imports.
In `@src/dao/browser/import/dao_profile_snapshot.cc`:
- Around line 198-210: Update the optional sidecar-copy handling in the snapshot
import flow around CopyStableFile so kPermissionDenied, kChanging, and other
non-cancellation outcomes do not set result.error_code or fail the snapshot.
Abort and return only when the copy outcome indicates cancellation; otherwise
continue processing the remaining sidecar files and preserve the successful
main-database copy.
In `@src/dao/browser/ui/webui/resources/import/dao_import_app.ts`:
- Around line 779-801: Correct the ARIA semantics in the source card list:
update the elements generated by sourceCards_() so the listbox container has
children with role="option", preserving aria-selected and the existing
selection/click behavior. Do not retain the implicit button role alongside the
listbox option role.
- Around line 885-890: Update renderTask_ to validate item.phase and category
against their known values before performing localization lookups. Replace the
unsafe item.phase[0] access with a guarded localized generic fallback for empty
or unknown phases, and apply the same known-value guard to category lookups
before calling loadTimeData.getString.
In `@src/dao/browser/ui/webui/resources/import/import.html`:
- Around line 1-2: Update the root html element in the import document to
include the WebUI-replaced lang="$i18n{language}" and dir="$i18n{textdirection}"
attributes, preserving the existing document structure.
In `@src/dao/browser/ui/webui/resources/import/import.ts`:
- Around line 11-15: Remove the identity `default` Trusted Types policy created
near `trustedWindow.trustedTypes`, including its `createHTML`, `createScript`,
and `createScriptURL` handlers. Rely on Lit’s existing `lit-html-desktop` policy
allowed by CSP, without introducing another unsanitized fallback.
In `@src/dao/browser/ui/webui/resources/sidebar/dao_sidebar_app.ts`:
- Around line 478-489: Update reloadFolders_() so it loads the folder data and
marks folders as loaded without calling saveFolders_() after reconcile. Remove
the persistence from this reload path, allowing the next sidebarStateChanged
handler to reconcile against the current unpinnedTabs_ and save the resulting
model.
---
Nitpick comments:
In `@scripts/commands/__tests__/browser_import_contract.test.ts`:
- Around line 22-24: Relax the regex assertion in the browser import contract
test so it validates that the same CreateSequencedTaskRunner call contains both
base::MayBlock() and base::TaskShutdownBehavior::BLOCK_SHUTDOWN, without
depending on trait ordering, whitespace, line breaks, or comments.
In `@src/dao/browser/import/dao_chromium_migration_target.cc`:
- Around line 160-193: No code change is required: retain AddHistoryVisit and
OnHistoryWriteVerified as-is, since visit persistence is the required
verification for this merge-only import and title verification is not required.
- Around line 278-298: Update DaoChromiumMigrationTarget::IsTabOpen to recognize
already-open tabs when the imported URL differs due to redirects or
trailing-slash normalization. Compare url against each WebContents object's
GetLastCommittedURL() in addition to GetVisibleURL(), preserving the existing
early-exit behavior when either URL matches.
- Around line 431-441: Update
DaoChromiumMigrationTarget::CancelExtensionInstalls so the active-entry
preservation branch runs only when extension_installer_ is non-null and
extension_queue_ is non-empty; otherwise clear the queue as currently done. Keep
MaybeFinishExtensionInstalls() invoked after either path.
In `@src/dao/browser/import/dao_chromium_password_decryptor_mac.h`:
- Around line 8-12: 补齐头文件的直接依赖：在 dao_chromium_password_decryptor_mac.h 中显式包含
base/containers/span.h、<string> 和 <cstdint>，以支持文件直接使用的 base::span、std::u16string
与 uint8_t；保留现有 include，并不要依赖 dao_source_adapter.h 的传递引入。
In `@src/dao/browser/import/dao_chromium_password_decryptor_mac.mm`:
- Around line 88-98: Update LoadKey() to use
crypto::SecureZeroBuffer(base::span<uint8_t>) to clear the sensitive password
buffer and derived_key before every return, including failure paths. Add
equivalent secure clearing of the initialized key_ in the decryptor’s
destructor, avoiding ordinary memset and preserving the existing key derivation
behavior.
In `@src/dao/browser/import/dao_chromium_profile_adapter_unittest.cc`:
- Around line 7-17: 在测试文件的 include 区域补充直接使用符号所需的头文件：为 std::optional、std::vector
和 base::as_byte_span 分别显式引入对应声明，保留现有 include，并避免依赖传递引入。
In `@src/dao/browser/import/dao_chromium_profile_adapter.cc`:
- Around line 35-43: Update ChromiumTimeFromString to reject only negative
microsecond values by changing the validation boundary from <= 0 to < 0,
allowing zero timestamps to convert through
base::Time::FromDeltaSinceWindowsEpoch while preserving existing null-time
behavior for missing or unparsable input.
In `@src/dao/browser/import/dao_chromium_profile_adapter.h`:
- Around line 19-25: 统一按仓库 Chromium clang-format
风格格式化以下位置：src/dao/browser/import/dao_chromium_profile_adapter.h:19-25
将访问说明符缩进一格并采用类型紧邻引用符的写法；src/dao/browser/import/dao_migration_service.h:42-49
同样调整访问说明符及引用、指针绑定方式；src/dao/browser/import/dao_migration_service.cc:385-401 将
switch 的 case 标签缩进两格，并统一参数中的引用和指针格式。
In `@src/dao/browser/import/dao_legacy_profile_writer.cc`:
- Around line 54-68: Update DaoLegacyProfileWriter::AddHistoryPage to append
each constructed HistoryVisit directly to pending_history_ while iterating over
page, removing the temporary visits vector, its reserve call, and the subsequent
move-insert.
In `@src/dao/browser/import/dao_legacy_profile_writer.h`:
- Around line 8-14: 在 import 相关声明中补充直接依赖的 <vector> 和 <cstddef> 头文件，确保
dao_legacy_profile_writer.h 中使用的 std::vector 与 size_t 可独立编译；保留现有
include，并按项目规范整理新增 include。
In `@src/dao/browser/import/dao_migration_job_unittest.cc`:
- Around line 109-131: Add focused tests to DaoMigrationJobTest covering the
missing state-machine guards: reject StartCategory when another category is
running and after RequestCancel(), verify CancelRunningCategoryAtBatchBoundary
returns false without cancellation being requested, and verify UpdateProgress
with total_items equal to zero sets indeterminate to true. Reuse the existing
TestSource, categories, and state assertions used by
CancelsPendingWorkAtSafeBatchBoundary.
In `@src/dao/browser/import/dao_migration_job.cc`:
- Around line 14-21: Update DaoMigrationJob and DaoMigrationService::Start to
handle empty selections explicitly, either rejecting them in Start or
documenting the intentional immediate-terminal behavior in the constructor. In
DaoMigrationJob::DaoMigrationJob, deduplicate selected_categories_ before
populating category_states_ so GetState() and the map maintain consistent
category counts.
In `@src/dao/browser/import/dao_migration_job.h`:
- Around line 8-13: Add the direct <cstdint> include to dao_migration_job.h,
alongside the existing standard-library includes, so UpdateProgress can use
uint64_t without relying on dao_migration_types.h's transitive includes.
In `@src/dao/browser/import/dao_migration_service_factory.cc`:
- Around line 24-27: 在 DaoMigrationServiceFactory::DaoMigrationServiceFactory
构造函数中，为
BookmarkModelFactory、HistoryServiceFactory、ProfilePasswordStoreFactory、extensions::ExtensionRegistryFactory
和 extensions::ExtensionRegistrarFactory 调用 DependsOn 进行依赖声明；不要将非 keyed service 的
ProfileBrowserCollection 加入依赖列表。
In `@src/dao/browser/import/dao_migration_service_factory.h`:
- Around line 8-9: 在头文件的 include 区域补充标准库头文件 <memory>，以便声明或使用 std::unique_ptr
的代码具备直接依赖；保留现有 include 不变，并确认该文件的所有 include 依赖完整。
- Around line 17-31: 将 DaoMigrationServiceFactory 改为继承
ProfileKeyedServiceFactory，并使用默认的 “DaoMigrationService” 配置管理 regular profile；删除
GetBrowserContextToUse 声明及其实现，保留现有服务构造和获取接口。若代码支持 guest 或 incognito，改用
ProfileSelections 显式指定策略。
In `@src/dao/browser/import/dao_migration_service.cc`:
- Around line 684-692: 在处理 DataCategory::kExtensions 的分支中，调用
DaoMigrationService::target_->FinishExtensionInstalls 前先判空 target_；若 target_ 已被
Shutdown() 重置，则直接返回并保持现有流程不变。
- Around line 511-519: Update the category handling in the migration loop around
LegacyImportItem and StartCategory so an item value of 0 explicitly marks the
category as failed before returning, rather than leaving it pending. Preserve
the existing StartCategory failure behavior for valid legacy items, and use the
existing category failure mechanism.
- Around line 158-167: 在 DaoMigrationService 中更新 kTabs 的计数快照创建逻辑，将创建的
SnapshotCancellationFlag 同时保存到成员变量 snapshot_cancellation_，而不是仅保留在局部
request.cancellation 中。确保 Cancel() 能通过该成员取消计数阶段的标签页快照，并与迁移阶段共用同一取消路径。
In `@src/dao/browser/import/dao_migration_service.h`:
- Around line 8-27: 在 dao_migration_service.h 的 include 区域显式添加 <utility> 和
base/files/file_path.h，以覆盖 std::pair 与 base::FilePath 的直接依赖；保留现有包含并按项目头文件排序规范排列。
In `@src/dao/browser/import/dao_migration_writer_unittest.cc`:
- Around line 97-100: Update the fake QueueExtensionInstall implementation to
honor an accept_extension_queue flag, returning false without recording the
extension when queueing is rejected. Add a WriteExtensions test configuring the
flag to reject the queue and assert that failed equals 1.
In `@src/dao/browser/import/dao_profile_snapshot.cc`:
- Around line 201-205: Update the sidecar-path construction in the loop around
CopyStableFile to use FILE_PATH_LITERAL-compatible path components instead of
std::string(suffix), preserving the "-wal" and "-shm" suffixes while supporting
both POSIX and Windows FilePath::StringType.
In `@src/dao/browser/import/dao_source_detector.h`:
- Around line 59-69: 为 DaoSourceDetector 增加 SEQUENCE_CHECKER 成员，保护仅在调用序列上访问的状态。
在 Detect、OnDetectionComplete 和 ResolveProfilePath 中执行序列检查，覆盖对 profile_paths_ 与
detection_generation_ 的读写；同时补充所需头文件并保持现有逻辑不变。
In `@src/dao/browser/ui/dao_ui_sources.gni`:
- Around line 208-211: Sort the GN resource lists in dao_ui_sources.gni
lexicographically, placing import before sidebar and moving the unittest entries
after the existing agent entry as appropriate. Keep each contiguous string list
consistently ordered so it matches gn format output.
In `@src/dao/browser/ui/webui/resources/import/__tests__/dao_import_app.test.ts`:
- Around line 279-303: Update the test around createApp and the generated style
text to assert directly against cssText, verifying the completion selector does
not assign 390px min-height to completed rail segments. Remove the
getComputedStyle-based assertion and keep the check deterministic through the
stylesheet declaration text.
In `@src/dao/browser/ui/webui/resources/import/dao_import_app.ts`:
- Around line 646-648: Fix the unknown-source fallback in sourceInitial_ and its
call site so it receives the actual source kind instead of the empty string,
ensuring the fallback glyph is non-empty. Preserve the existing fallback
rendering behavior and remove the helper only if the real kind cannot be passed
through.
In `@src/dao/browser/ui/webui/resources/import/import_bridge.ts`:
- Around line 53-85: Update sendAsync to reject and remove its callback listener
when no native response arrives within a timeout, while preserving the existing
callback-event protocol through chrome.send. Ensure successful responses clear
the timeout before resolving, and timeout handling removes the listener before
rejecting.
In `@src/patches/chrome/browser/ui/webui/chrome_web_ui_configs.cc.patch`:
- Around line 38-42: Remove the commented-out skills::SkillsUIConfig
registration from the WebUI configuration patch, keeping the existing
explanatory comment about DaoSkillsUIConfig. Check whether the upstream
SkillsUIConfig include is now unused and remove it if so.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3335cb8-f55f-426a-9b20-38df3672cb05
⛔ Files ignored due to path filters (5)
src/dao/browser/ui/webui/resources/import/assets/arc.svgis excluded by!**/*.svgsrc/dao/browser/ui/webui/resources/import/assets/chrome.svgis excluded by!**/*.svgsrc/dao/browser/ui/webui/resources/import/assets/edge.svgis excluded by!**/*.svgsrc/dao/browser/ui/webui/resources/import/assets/firefox.svgis excluded by!**/*.svgsrc/dao/browser/ui/webui/resources/import/assets/safari.svgis excluded by!**/*.svg
📒 Files selected for processing (62)
design-qa.mddocs/feature-checklist.mddocs/features.mdscripts/commands/__tests__/browser_import_contract.test.tsscripts/commands/__tests__/settings_redesign_contract.test.tssrc/dao/browser/import/dao_chromium_migration_target.ccsrc/dao/browser/import/dao_chromium_migration_target.hsrc/dao/browser/import/dao_chromium_password_decryptor_mac.hsrc/dao/browser/import/dao_chromium_password_decryptor_mac.mmsrc/dao/browser/import/dao_chromium_profile_adapter.ccsrc/dao/browser/import/dao_chromium_profile_adapter.hsrc/dao/browser/import/dao_chromium_profile_adapter_unittest.ccsrc/dao/browser/import/dao_legacy_profile_writer.ccsrc/dao/browser/import/dao_legacy_profile_writer.hsrc/dao/browser/import/dao_migration_job.ccsrc/dao/browser/import/dao_migration_job.hsrc/dao/browser/import/dao_migration_job_unittest.ccsrc/dao/browser/import/dao_migration_service.ccsrc/dao/browser/import/dao_migration_service.hsrc/dao/browser/import/dao_migration_service_factory.ccsrc/dao/browser/import/dao_migration_service_factory.hsrc/dao/browser/import/dao_migration_types.ccsrc/dao/browser/import/dao_migration_types.hsrc/dao/browser/import/dao_migration_writer.ccsrc/dao/browser/import/dao_migration_writer.hsrc/dao/browser/import/dao_migration_writer_unittest.ccsrc/dao/browser/import/dao_profile_snapshot.ccsrc/dao/browser/import/dao_profile_snapshot.hsrc/dao/browser/import/dao_profile_snapshot_unittest.ccsrc/dao/browser/import/dao_source_adapter.ccsrc/dao/browser/import/dao_source_adapter.hsrc/dao/browser/import/dao_source_detector.ccsrc/dao/browser/import/dao_source_detector.hsrc/dao/browser/import/dao_source_detector_unittest.ccsrc/dao/browser/strings/dao_strings.grdsrc/dao/browser/strings/translations/dao_strings_zh-CN.xtbsrc/dao/browser/ui/dao_ui_sources.gnisrc/dao/browser/ui/views/dao_browser_browsertest.ccsrc/dao/browser/ui/webui/dao_import_ui.ccsrc/dao/browser/ui/webui/dao_import_ui.hsrc/dao/browser/ui/webui/dao_sidebar_ui.ccsrc/dao/browser/ui/webui/dao_sidebar_ui.hsrc/dao/browser/ui/webui/resources/import/BUILD.gnsrc/dao/browser/ui/webui/resources/import/__tests__/dao_import_app.test.tssrc/dao/browser/ui/webui/resources/import/dao_import_app.tssrc/dao/browser/ui/webui/resources/import/import.csssrc/dao/browser/ui/webui/resources/import/import.htmlsrc/dao/browser/ui/webui/resources/import/import.tssrc/dao/browser/ui/webui/resources/import/import_bridge.tssrc/dao/browser/ui/webui/resources/sidebar/dao_sidebar_app.tssrc/patches/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc.patchsrc/patches/chrome/browser/resources/settings/dao_page/dao_page.html.patchsrc/patches/chrome/browser/resources/settings/dao_page/dao_page.ts.patchsrc/patches/chrome/browser/resources/settings/people_page/people_page.ts.patchsrc/patches/chrome/browser/ui/chrome_pages.cc.patchsrc/patches/chrome/browser/ui/webui/chrome_web_ui_configs.cc.patchsrc/patches/chrome/chrome_paks.gni.patchsrc/patches/chrome/test/data/webui/settings/dao_page_test.ts.patchsrc/patches/crypto/subtle_passkey.h.patchsrc/patches/third_party/lit/v3_0/BUILD.gn.patchsrc/patches/tools/gritsettings/resource_ids.spec.patchsrc/patches/tools/metrics/histograms/metadata/sql/histograms.xml.patch
| - User-reported broken state: `/var/folders/0l/4dc990md3yn_g3b46dtmhp880000gn/T/orca-paste-1786372826229-081b4a4d-c6af-4262-8884-3f09e49acc37.png` | ||
| - Source pixels: 1940 x 1610. | ||
| - State: dark theme, migration step 2, category selection screen. | ||
|
|
||
| **Implementation screenshot path** | ||
| ## Implementation evidence | ||
|
|
||
| Unavailable. The required in-app browser is not available in this session, and the project cannot produce a fresh Dao binary because the shared generated Chromium checkout fails normal import on 15 unrelated Settings patches before compilation begins. | ||
| - Browser-rendered screenshot: `/var/folders/0l/4dc990md3yn_g3b46dtmhp880000gn/T/orca-computer-use/1c00cbd7-8d01-4d07-b35d-db9c98d4b199-screenshot.png` | ||
| - Before/after comparison: `/tmp/dao-import-layout-before-after.png` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace machine-local artifact paths.
The report links to /var/folders/... and /tmp/... files that other reviewers and CI cannot access. Store required screenshots as repository-relative artifacts or attach them through the supported QA artifact system, and reference stable paths.
Also applies to: 65-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-qa.md` around lines 5 - 12, Replace the machine-local screenshot
references in the design-qa report with repository-relative artifact paths or
supported QA attachment references. Update both the user-reported broken-state
path and the implementation-evidence comparison path, while preserving the
documented screenshot context.
| - **Merge-only destination writes** — Bookmarks are placed under a localized | ||
| imported root, destination password conflicts are preserved, already-open | ||
| tab URLs and installed extensions are skipped, and history writes use the | ||
| profile History service. History and password counts advance only after the | ||
| destination services confirm the persisted records. No category replaces | ||
| existing Dao data. | ||
| - **Passwords and extensions** — The selection screen warns that password | ||
| decryption may trigger a macOS Keychain authorization prompt. A denial fails | ||
| only passwords. Compatible web-store extensions are reinstalled in sequence; | ||
| extension storage and sign-in state are not copied. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
澄清扩展跳过条件。
Line 382 says “installed extensions are skipped”, while Line 388 says compatible web-store extensions are reinstalled. Specify that extensions already installed in the destination are skipped. Keep source web-store extensions eligible for sequential reinstallation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/features.md` around lines 380 - 389, Clarify the “Merge-only destination
writes” documentation to state that extensions already installed in the
destination are skipped. Keep the “Passwords and extensions” description
explicit that compatible source web-store extensions remain eligible for
sequential reinstallation.
| std::string DaoChromiumMigrationTarget::EnsureImportedTabFolder( | ||
| const std::u16string& folder_name) { | ||
| if (!LoadFolderData()) { | ||
| return std::string(); | ||
| } | ||
| const std::string id = base::Uuid::GenerateRandomV4().AsLowercaseString(); | ||
| base::DictValue folder; | ||
| folder.Set("type", "folder"); | ||
| folder.Set("id", id); | ||
| folder.Set("name", base::UTF16ToUTF8(folder_name)); | ||
| folder.Set("collapsed", true); | ||
| folder.Set("children", base::ListValue()); | ||
| folder_items_->Append(std::move(folder)); | ||
| pending_folder_ = folder_items_->back().GetIfDict(); | ||
| return id; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
pending_folder_ and folder_items_ can dangle across a second EnsureImportedTabFolder call.
LoadFolderData() move-assigns folder_data_ and re-resolves folder_items_. pending_folder_ then points into the previous base::Value tree. folder_items_->Append() can also reallocate the list storage and invalidate pending_folder_, which points at an element of the same list.
Both pointers are raw_ptr<>, so a stale access is a crash in a MiraclePtr build, not silent corruption. A second call while a folder is still pending also leaves pending_folder_tab_ids_ populated with tabs from the earlier folder, so AbortImportedTabFolder would close the wrong tabs.
Store the pending folder by its id and look it up in folder_items_ on each use, or reject a second EnsureImportedTabFolder call while pending_folder_ is set and clear pending_folder_tab_ids_ at the start of the call.
🛡️ Minimal guard
std::string DaoChromiumMigrationTarget::EnsureImportedTabFolder(
const std::u16string& folder_name) {
+ if (pending_folder_) {
+ return std::string();
+ }
if (!LoadFolderData()) {
return std::string();
}
+ pending_folder_tab_ids_.clear();
const std::string id = base::Uuid::GenerateRandomV4().AsLowercaseString();Also applies to: 554-574
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/import/dao_chromium_migration_target.cc` around lines 300 -
315, Update EnsureImportedTabFolder and the pending-folder handling to prevent
stale pointers after LoadFolderData or folder_items_ reallocation. Reject a
second EnsureImportedTabFolder call while a folder is pending, and clear
pending_folder_tab_ids_ when starting a new valid import; ensure the existing
pending-folder state remains consistent for AbortImportedTabFolder.
| std::string DaoChromiumMigrationTarget::BookmarkKey( | ||
| const BookmarkEntry& entry) const { | ||
| return JoinPath(entry.path) + base::UTF16ToUTF8(entry.title) + "\0" + | ||
| entry.url.spec(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
BookmarkKey loses its separator; + "\0" appends nothing.
operator+ with the string literal "\0" treats the argument as a NUL-terminated C string, so it appends an empty string. The title and the URL are concatenated with no delimiter. Two different bookmarks can then produce the same key, so HasBookmark can report a false duplicate and AddBookmark can skip a real entry.
Note that JoinPath uses push_back('\0'), which does append the NUL byte, so the intent is clear.
🐛 Proposed fix
std::string DaoChromiumMigrationTarget::BookmarkKey(
const BookmarkEntry& entry) const {
- return JoinPath(entry.path) + base::UTF16ToUTF8(entry.title) + "\0" +
- entry.url.spec();
+ std::string key = JoinPath(entry.path);
+ key.append(base::UTF16ToUTF8(entry.title));
+ key.push_back('\0');
+ key.append(entry.url.spec());
+ return key;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string DaoChromiumMigrationTarget::BookmarkKey( | |
| const BookmarkEntry& entry) const { | |
| return JoinPath(entry.path) + base::UTF16ToUTF8(entry.title) + "\0" + | |
| entry.url.spec(); | |
| } | |
| std::string DaoChromiumMigrationTarget::BookmarkKey( | |
| const BookmarkEntry& entry) const { | |
| std::string key = JoinPath(entry.path); | |
| key.append(base::UTF16ToUTF8(entry.title)); | |
| key.push_back('\0'); | |
| key.append(entry.url.spec()); | |
| return key; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/import/dao_chromium_migration_target.cc` around lines 506 -
510, Update DaoChromiumMigrationTarget::BookmarkKey to append an actual NUL
separator between entry.title and entry.url.spec(), using an operation that
preserves the byte rather than the NUL-terminated "\0" literal. Keep the
existing JoinPath and key components unchanged.
| ReadBatch<HistoryVisit> DaoChromiumProfileAdapter::ReadHistory() { | ||
| sql::Database database(kDatabaseTag); | ||
| if (!database.Open(profile_path_.AppendASCII("History"))) { | ||
| return ReadFailure<HistoryVisit>("sqlite_open_failed"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Open the source databases read-only.
CountCandidates opens History and Login Data with sql::DatabaseOptions().set_read_only(true), but ReadHistory and ReadPasswords open the same files read-write. A read-write open can create or modify journal/WAL files next to the source database. Use the same read-only options in both read paths.
♻️ Proposed change
- sql::Database database(kDatabaseTag);
+ sql::Database database(sql::DatabaseOptions().set_read_only(true),
+ kDatabaseTag);
if (!database.Open(profile_path_.AppendASCII("History"))) {- sql::Database database(kDatabaseTag);
+ sql::Database database(sql::DatabaseOptions().set_read_only(true),
+ kDatabaseTag);
if (!database.Open(profile_path_.AppendASCII("Login Data"))) {Also applies to: 180-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/import/dao_chromium_profile_adapter.cc` around lines 144 -
148, Update ReadHistory and ReadPasswords to open their source databases using
sql::DatabaseOptions().set_read_only(true), matching CountCandidates. Preserve
the existing failure handling and database paths while preventing read paths
from opening the files read-write.
| <div class="source-grid" role="listbox"> | ||
| ${this.sourceCards_().map(card => { | ||
| const source = card.source; | ||
| return html` | ||
| <button class="source-card ${ | ||
| source?.id === this.selectedSourceId_ ? 'selected' : ''}" | ||
| data-source-kind=${card.kind} | ||
| data-source-id=${source?.id || nothing} | ||
| aria-selected="${source?.id === this.selectedSourceId_}" | ||
| .disabled=${!source} | ||
| @click=${() => source && this.selectSource_(source)}> | ||
| <span class="source-mark ${card.kind}" aria-hidden="true"> | ||
| <img class="source-logo" src=${this.sourceLogo_(card.kind)} | ||
| alt=""> | ||
| </span> | ||
| <span class="card-copy"> | ||
| <span class="card-title">${card.browserName}</span> | ||
| <span class="card-meta">${source?.profileName || | ||
| this.string_('daoImportSourceNotDetected')}</span> | ||
| </span> | ||
| <span class="selection" aria-hidden="true"></span> | ||
| </button>`; | ||
| })} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the listbox ARIA roles.
The container declares role="listbox", but each child is a <button> with an implicit button role. aria-selected is not valid on a button, and a listbox requires children with role="option". Screen readers therefore announce the selection state incorrectly.
Add role="option" to each card, or drop role="listbox" and use aria-pressed on the buttons instead.
♿ Proposed fix
<button class="source-card ${
source?.id === this.selectedSourceId_ ? 'selected' : ''}"
+ role="option"
data-source-kind=${card.kind}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="source-grid" role="listbox"> | |
| ${this.sourceCards_().map(card => { | |
| const source = card.source; | |
| return html` | |
| <button class="source-card ${ | |
| source?.id === this.selectedSourceId_ ? 'selected' : ''}" | |
| data-source-kind=${card.kind} | |
| data-source-id=${source?.id || nothing} | |
| aria-selected="${source?.id === this.selectedSourceId_}" | |
| .disabled=${!source} | |
| @click=${() => source && this.selectSource_(source)}> | |
| <span class="source-mark ${card.kind}" aria-hidden="true"> | |
| <img class="source-logo" src=${this.sourceLogo_(card.kind)} | |
| alt=""> | |
| </span> | |
| <span class="card-copy"> | |
| <span class="card-title">${card.browserName}</span> | |
| <span class="card-meta">${source?.profileName || | |
| this.string_('daoImportSourceNotDetected')}</span> | |
| </span> | |
| <span class="selection" aria-hidden="true"></span> | |
| </button>`; | |
| })} | |
| <div class="source-grid" role="listbox"> | |
| ${this.sourceCards_().map(card => { | |
| const source = card.source; | |
| return html` | |
| <button class="source-card ${ | |
| source?.id === this.selectedSourceId_ ? 'selected' : ''}" | |
| role="option" | |
| data-source-kind=${card.kind} | |
| data-source-id=${source?.id || nothing} | |
| aria-selected="${source?.id === this.selectedSourceId_}" | |
| .disabled=${!source} | |
| @click=${() => source && this.selectSource_(source)}> | |
| <span class="source-mark ${card.kind}" aria-hidden="true"> | |
| <img class="source-logo" src=${this.sourceLogo_(card.kind)} | |
| alt=""> | |
| </span> | |
| <span class="card-copy"> | |
| <span class="card-title">${card.browserName}</span> | |
| <span class="card-meta">${source?.profileName || | |
| this.string_('daoImportSourceNotDetected')}</span> | |
| </span> | |
| <span class="selection" aria-hidden="true"></span> | |
| </button>`; | |
| })} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/ui/webui/resources/import/dao_import_app.ts` around lines 779
- 801, Correct the ARIA semantics in the source card list: update the elements
generated by sourceCards_() so the listbox container has children with
role="option", preserving aria-selected and the existing selection/click
behavior. Do not retain the implicit button role alongside the listbox option
role.
| private renderTask_(item: CategoryState) { | ||
| const running = ['snapshotting', 'reading', 'writing'].includes(item.phase); | ||
| const count = item.totalItems > 0 ? | ||
| `${item.completedItems} / ${item.totalItems}` : | ||
| this.string_(`daoImportPhase${ | ||
| item.phase[0]!.toUpperCase()}${item.phase.slice(1)}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List the phase and category identifiers the backend can emit and compare them with the localized keys.
rg -n -C3 'phase|Phase' src/dao/browser/import/dao_migration_types.h src/dao/browser/import/dao_migration_types.cc
rg -n 'daoImportPhase|daoImportCategory' src/dao/browser/ui/webui/dao_import_ui.ccRepository: msgbyte/dao-browser
Length of output: 4774
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- WebUI structure ---'
ast-grep outline src/dao/browser/ui/webui/resources/import/dao_import_app.ts
echo '--- Relevant render and key construction ---'
rg -n -C8 'renderTask_|daoImportPhase|daoImportCategory|category' \
src/dao/browser/ui/webui/resources/import/dao_import_app.ts
echo '--- Backend state serialization ---'
rg -n -C6 'CategoryPhaseToString|DataCategoryToString|phase|category' \
src/dao/browser/ui/webui src/dao/browser/import \
-g '*.cc' -g '*.h' -g '*.ts'
echo '--- Localization entries ---'
rg -n -C2 'IDS_DAO_IMPORT_(PHASE|CATEGORY)|daoImport(Phase|Category)' \
src/dao/browser/strings src/dao/browser/ui/webui -g '*'Repository: msgbyte/dao-browser
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- Runtime key construction in dao_import_app.ts ---'
nl -ba src/dao/browser/ui/webui/resources/import/dao_import_app.ts |
sed -n '620,660p;870,910p'
echo '--- Relevant type definitions and serializers ---'
nl -ba src/dao/browser/import/dao_migration_types.h | sed -n '1,105p'
nl -ba src/dao/browser/import/dao_migration_types.cc | sed -n '1,90p'
echo '--- Backend-to-WebUI conversion sites ---'
rg -n -C8 --glob '*.cc' --glob '*.h' \
'CategoryPhaseToString|DataCategoryToString|completedItems|totalItems|phase.*category|category.*phase' \
src/dao/browser/import src/dao/browser/ui/webui
echo '--- Exact localized key registrations ---'
sed -n '250,305p' src/dao/browser/ui/webui/dao_import_ui.ccRepository: msgbyte/dao-browser
Length of output: 21256
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- Frontend lines 630-655 ---'
sed -n '630,655p' src/dao/browser/ui/webui/resources/import/dao_import_app.ts
echo '--- Frontend lines 878-900 ---'
sed -n '878,900p' src/dao/browser/ui/webui/resources/import/dao_import_app.ts
echo '--- Frontend type and state flow ---'
rg -n -C5 'interface CategoryState|type CategoryState|categories|renderTask_|renderCategory|phase:' \
src/dao/browser/ui/webui/resources/import/dao_import_app.ts \
src/dao/browser/ui/webui/resources/import -g '*.ts'
echo '--- Enum declarations ---'
sed -n '1,95p' src/dao/browser/import/dao_migration_types.h
sed -n '41,85p' src/dao/browser/import/dao_migration_types.ccRepository: msgbyte/dao-browser
Length of output: 39795
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- loadTimeData.getString semantics in repository sources ---'
rg -n -C8 'getString\s*\(' src engine -g '*.ts' -g '*.js' -g '*.cc' -g '*.h' 2>/dev/null |
head -n 220
echo '--- Generic import labels available for fallback ---'
rg -n -C2 'daoImport.*(Unknown|Generic|Status|Phase|Category|Unavailable|Fallback)' \
src/dao/browser/strings src/dao/browser/ui/webui -g '*'
echo '--- JavaScript edge-case behavior ---'
node - <<'JS'
for (const phase of ['', 'futurePhase']) {
try {
const key = `daoImportPhase${phase[0].toUpperCase()}${phase.slice(1)}`;
console.log(JSON.stringify({phase, key}));
} catch (error) {
console.log(JSON.stringify({phase, error: String(error)}));
}
}
try {
''.toUpperCase();
console.log('empty-string-method:ok');
} catch (error) {
console.log(`empty-string-method:error:${String(error)}`);
}
try {
const first = undefined;
first.toUpperCase();
} catch (error) {
console.log(`undefined-toUpperCase:error:${String(error)}`);
}
JSRepository: msgbyte/dao-browser
Length of output: 16652
Guard phase and category localization lookups.
Map item.phase and category through known values before calling loadTimeData.getString. Use a localized generic fallback for unknown or empty values. The empty phase currently throws because the non-null assertion does not protect the runtime toUpperCase() call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/ui/webui/resources/import/dao_import_app.ts` around lines 885
- 890, Update renderTask_ to validate item.phase and category against their
known values before performing localization lookups. Replace the unsafe
item.phase[0] access with a guarded localized generic fallback for empty or
unknown phases, and apply the same known-value guard to category lookups before
calling loadTimeData.getString.
| <!DOCTYPE html> | ||
| <html> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how other Dao WebUI pages declare lang/dir and whether i18n replacement is enabled.
fd -e html . src/dao/browser/ui/webui/resources --exec rg -n '<html' {} +
rg -n 'UseStringsJs|EnableReplaceI18nInJS|AddLocalizedString' src/dao/browser/ui/webui/dao_import_ui.ccRepository: msgbyte/dao-browser
Length of output: 1092
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- HTML language declarations ---'
for f in $(fd -e html . src/dao/browser/ui/webui/resources); do
printf '\n[%s]\n' "$f"
rg -n -C 2 '<html|i18n|loadTimeData' "$f" || true
done
printf '%s\n' '--- dao_import_ui.cc localization setup ---'
sed -n '220,330p' src/dao/browser/ui/webui/dao_import_ui.cc
printf '%s\n' '--- import resource registration and template usage ---'
rg -n -C 3 'import\.html|AddResourcePath|Set.*DataSource|Replace|i18n|UseStringsJs|AddLocalizedStrings' src/dao/browser/ui/webui src/dao/browser/strings
printf '%s\n' '--- import files ---'
fd -i 'import' src/dao/browser/ui/webui/resources --type fRepository: msgbyte/dao-browser
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- import.html ---'
cat -n src/dao/browser/ui/webui/resources/import/import.html
printf '%s\n' '--- import TypeScript files ---'
fd -i -t f 'import' src/dao/browser/ui/webui/resources/import
for f in $(fd -i -t f 'import' src/dao/browser/ui/webui/resources/import); do
printf '\n[%s]\n' "$f"
wc -l "$f"
rg -n -C 2 'loadTimeData|i18n|document|dir|language' "$f" || true
done
printf '%s\n' '--- dao_import_ui.cc localization and resource setup ---'
rg -n -C 8 'AddLocalizedStrings|UseStringsJs|WebUIDataSource|import\.html|AddResourcePath|SetDefaultResource' \
src/dao/browser/ui/webui/dao_import_ui.cc
printf '%s\n' '--- relevant Dao string identifiers ---'
rg -n -C 3 'textdirection|language|Import' src/dao/browser/strings/dao_strings.grdRepository: msgbyte/dao-browser
Length of output: 10406
🌐 Web query:
Chromium WebUIDataSource AddLocalizedStrings HTML $i18n{textdirection} $i18n{language} built-in
💡 Result:
In Chromium WebUI, the $i18n{key} syntax is a template replacement mechanism used to inject localized strings or configuration values directly into HTML files at request time [1][2]. Mechanism and Usage: - Replacements: When a WebUI page is requested, the C++ WebUIDataSource processes the HTML file and replaces tokens matching the $i18n{key} pattern with values registered in the data source [1][2]. - Configuration: You register these keys in C++ using WebUIDataSource::AddLocalizedString (for individual strings linked to GRD IDs) or WebUIDataSource::AddLocalizedStrings (for bulk additions) [1][3][2]. - Built-ins: While you can define custom keys, Chromium provides built-in or standard keys for common UI properties. Specifically, $i18n{textdirection} (which typically resolves to 'ltr' or 'rtl') and $i18n{language} (often used for locale-specific attributes) are commonly used to configure the root <html> element dynamically [3][4][5]. Example Implementation: In your HTML file, you can utilize these built-ins as follows: ... In your C++ WebUIController, ensure these values are populated in the WebUIDataSource [3][2]. While many localized strings are loaded via GRD files, these specific configuration values are often added via AddString or similar methods depending on the specific implementation requirements of your component [3][2]. Important Considerations: - Default Behavior: By default, $i18n{} escapes strings for HTML safety [1][2]. - Alternatives: For translations that must embed HTML, use $i18nRaw{} [1][2]. For dynamic interaction within Polymer or Lit components, developers often prefer the I18nMixin or I18nMixinLit, which provide access to this.i18n() methods in JavaScript/TypeScript rather than static HTML replacement [6][7][8]. - Lifecycle: This template replacement occurs on the server side (C++), whereas I18nMixin methods handle localization client-side [7][2].
Citations:
- 1: https://chromium.googlesource.com/chromium/src.git/+/d2e718e05d222bfb1c7df8e462b0eb7ca4f4c202/docs/webui_explainer.md
- 2: https://chromium.googlesource.com/chromium/src/+/main/docs/webui/webui_explainer.md
- 3: https://chromium.googlesource.com/chromium/src/+/9e6fdd36f7268b875a9a4dd6d8c94c500d200a21/docs/webui_in_components.md
- 4: https://chromium.googlesource.com/chromium/src/+/125.0.6422.112/docs/webui_in_components.md
- 5: https://chromium.googlesource.com/chromium/src/+/118.0.5993.117/docs/webui_in_components.md
- 6: https://www.chromium.org/chromium-os/developer-library/guides/localization/creating-and-translating-strings/
- 7: https://chromium.googlesource.com/chromium/src/+/ea36ef4e8dca185571d800c54d8d7fd4dd0a395d/ui/webui/resources/cr_elements/i18n_mixin.ts
- 8: https://chromium.googlesource.com/chromium/src/+/main/ui/webui/resources/cr_elements/i18n_mixin_lit.ts
🌐 Web query:
site:chromium.googlesource.com/chromium/src WebUIDataSource textdirection language AddLocalizedString
💡 Result:
In Chromium WebUI development, AddLocalizedString is a method used to register localized strings with a WebUIDataSource so they can be accessed in HTML and JavaScript [1][2][3]. When building WebUI pages, developers often need to support multiple languages and text directions (left-to-right or right-to-left). The text direction and language configuration are typically handled by injecting specific variables into the WebUI data source, which are then referenced in the HTML template [4][5][6]. 1. Registering Localized Strings: You use AddLocalizedString(name, ids) to associate a key name (used in JS/HTML) with a resource ID (an integer reference to a string in a.grd or.grdp file) [3]. For multiple strings, AddLocalizedStrings is more efficient [7][3]. 2. Handling Text Direction and Language: To support localization and RTL layout, the HTML document is typically configured with I18n placeholders [5][6]. The WebUIDataSource automatically handles these standard values if set up correctly. Specifically, you will often see templates like: [4][5][6] In your C++ WebUI controller code, the data source populates these values so that the $i18n{} syntax in your HTML replaces them at runtime with the appropriate localized settings for the user's current locale [4][3][5]. The Chromium Web Development Style Guide explicitly recommends setting the dir attribute to the 'textdirection' value to enable correct RTL/LTR flipping and to support CSS selectors like html[dir=rtl] [5].
Citations:
- 1: https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/content/browser/webui/web_ui_data_source_impl.h
- 2: https://chromium.googlesource.com/chromium/src/+/HEAD/content/browser/webui/web_ui_data_source_impl.h
- 3: https://chromium.googlesource.com/chromium/src/+/2dd666cdae6058aeabeadcdeba7faa168523ca0a/docs/webui_explainer.md
- 4: https://chromium.googlesource.com/chromium/src.git/+/d65a2097016a92391b624563edcebf4767173226%5E%21/
- 5: https://chromium.googlesource.com/chromium/src/+/5a8ca93a63dcd71811dc89f00538337dfbe9264a/styleguide/web/web.md
- 6: https://chromium.googlesource.com/chromium/src/+/aed207f466b2c5ed7d9c922ebf0d0b030d3d3bd4/docs/webui_in_components.md
- 7: https://chromium.googlesource.com/chromium/src/+show/3aa4fb98a44d997de209f76353f4a40c61a441a8/docs/webui_in_components.md
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from urllib.request import urlopen
urls = {
"web_ui_data_source_impl.cc":
"https://chromium.googlesource.com/chromium/src/+/main/content/browser/webui/web_ui_data_source_impl.cc?format=TEXT",
"web_ui_data_source.cc":
"https://chromium.googlesource.com/chromium/src/+/main/content/browser/webui/web_ui_data_source.cc?format=TEXT",
}
import base64
for name, url in urls.items():
print(f"--- {name}: matching lines ---")
data = base64.b64decode(urlopen(url, timeout=15).read()).decode("utf-8", "replace")
lines = data.splitlines()
for i, line in enumerate(lines):
if any(token in line for token in ("textdirection", "language", "ReplaceI18n", "localized_strings")):
start = max(0, i - 4)
end = min(len(lines), i + 5)
print("\n".join(f"{n+1}:{lines[n]}" for n in range(start, end)))
print()
PYRepository: msgbyte/dao-browser
Length of output: 2495
Set the document language and text direction.
Add dir="$i18n{textdirection}" and lang="$i18n{language}" to the root <html> element. The WebUI data source replaces these $i18n{} values in HTML.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/ui/webui/resources/import/import.html` around lines 1 - 2,
Update the root html element in the import document to include the
WebUI-replaced lang="$i18n{language}" and dir="$i18n{textdirection}" attributes,
preserving the existing document structure.
| trustedWindow.trustedTypes?.createPolicy('default', { | ||
| createHTML: (value: string) => value, | ||
| createScript: (value: string) => value, | ||
| createScriptURL: (value: string) => value, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The identity Trusted Types default policy removes the page protection.
createHTML, createScript, and createScriptURL return the input unchanged. Any string then reaches the DOM sinks, so the trusted-types default lit-html-desktop CSP directive set in src/dao/browser/ui/webui/dao_import_ui.cc no longer constrains this page. Migration data such as bookmark titles and profile names comes from external browser profiles, so it is untrusted input.
Lit creates its own lit-html-desktop policy, which the CSP already allows. Remove the default policy, or restrict it to the specific sink that needs it and sanitize the value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/ui/webui/resources/import/import.ts` around lines 11 - 15,
Remove the identity `default` Trusted Types policy created near
`trustedWindow.trustedTypes`, including its `createHTML`, `createScript`, and
`createScriptURL` handlers. Rely on Lit’s existing `lit-html-desktop` policy
allowed by CSP, without introducing another unsanitized fallback.
| private async reloadFolders_() { | ||
| try { | ||
| const json = await loadFolders(); | ||
| this.folderModel_.loadFromJson(json); | ||
| this.folderModel_.reconcile(this.unpinnedTabs_); | ||
| this.foldersLoaded_ = true; | ||
| this.saveFolders_(); | ||
| } catch (e) { | ||
| console.error('DaoSidebarApp: failed to reload folders', e); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
reloadFolders_() can delete the freshly imported folder in other sidebar windows.
reconcile() keeps a folder only when at least one child matches a tab in actualTabs, and it discards every unmatched entry (dao_folder_model.ts lines 329-411). reloadFolders_() then calls saveFolders_(), which persists the reduced model back to the shared profile file.
Two paths lose the imported folder:
DaoChromiumMigrationTarget::AddDormantTabadds the imported tabs to one browser window only. Every sidebar window of the profile receivesfolderDataChanged. In the other windows,this.unpinnedTabs_does not contain the imported tabs, so the imported folder is dropped and the deletion is written todao_folders.json.- If
folderDataChangedarrives before the firstsidebarStateChanged,this.unpinnedTabs_is empty and the whole model is emptied and saved.initFolders_()guards this case withif (this.unpinnedTabs_.length > 0);reloadFolders_()does not.
Reload without persisting, and let the next sidebarStateChanged handler perform reconciliation.
🐛 Proposed fix
private async reloadFolders_() {
try {
const json = await loadFolders();
this.folderModel_.loadFromJson(json);
- this.folderModel_.reconcile(this.unpinnedTabs_);
- this.foldersLoaded_ = true;
- this.saveFolders_();
+ this.foldersLoaded_ = true;
+ if (this.unpinnedTabs_.length > 0) {
+ this.folderModel_.reconcile(this.unpinnedTabs_);
+ }
+ // Do not persist here: the reloaded file is the source of truth and
+ // this window may not host the tabs that the folder references.
+ this.folderModelVersion_++;
} catch (e) {
console.error('DaoSidebarApp: failed to reload folders', e);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dao/browser/ui/webui/resources/sidebar/dao_sidebar_app.ts` around lines
478 - 489, Update reloadFolders_() so it loads the folder data and marks folders
as loaded without calling saveFolders_() after reconcile. Remove the persistence
from this reload path, allowing the next sidebarStateChanged handler to
reconcile against the current unpinnedTabs_ and save the resulting model.
1 participant
Background
Dao needs a safer way to move browser data into a local profile without replacing existing data or relying on Chromium's old modal importer.
Changes
dao://importWebUI for choosing source profiles, selecting data categories, tracking progress, cancelling, and retrying failed categories.Testing
Patch adds C++ unit tests for source detection, snapshots, profile adapters, migration jobs, and writers; WebUI tests for the import app; contract tests for browser import and Settings entry points; and browser tests for loading
dao://import, command routing, and tab rollback behavior.Summary by CodeRabbit